Re: make BuiltinTrancheNames less ugly

On Wed Jan 24, 2024 at 8:09 AM CST, Alvaro Herrera wrote:
> From 3d24b89855888a6650ec1aafb3579d810bfec5ac Mon Sep 17 00:00:00 2001
> From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> Date: Tue, 23 Jan 2024 10:36:14 +0100
> Subject: [PATCH] Remove IndividualLWLockNames
>
> We can just merge the lwlock names into the BuiltinTrancheNames array.
> This requires that Meson compiles the file with -I. in CPPFLAGS, which
> in turn requires some additional contortions for DTrace support in
> FreeBSD.
> ---
>  src/backend/meson.build                          |  4 +++-
>  src/backend/storage/lmgr/Makefile                |  3 ++-
>  src/backend/storage/lmgr/generate-lwlocknames.pl | 10 ++--------
>  src/backend/storage/lmgr/lwlock.c                | 13 ++++---------
>  src/backend/storage/lmgr/meson.build             | 12 ++++++++++--
>  5 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/src/backend/meson.build b/src/backend/meson.build
> index 8767aaba67..57a52c37e0 100644
> --- a/src/backend/meson.build
> +++ b/src/backend/meson.build
> @@ -127,7 +127,9 @@ backend_objs = [postgres_lib.extract_all_objects(recursive: false)]
>  if dtrace.found() and host_system != 'darwin'
>    backend_input += custom_target(
>      'probes.o',
> -    input: ['utils/probes.d', postgres_lib.extract_objects(backend_sources, timezone_sources)],
> +    input: ['utils/probes.d',
> +      postgres_lib.extract_objects(backend_sources, timezone_sources),
> +      lwlock_lib.extract_objects(lwlock_source)],
>      output: 'probes.o',
>      command: [dtrace, '-C', '-G', '-o', '@OUTPUT@', '-s', '@INPUT@'],
>      install: false,
> diff --git a/src/backend/storage/lmgr/Makefile b/src/backend/storage/lmgr/Makefile
> index 504480e847..81da6ee13a 100644
> --- a/src/backend/storage/lmgr/Makefile
> +++ b/src/backend/storage/lmgr/Makefile
> @@ -12,13 +12,14 @@ subdir = src/backend/storage/lmgr
>  top_builddir = ../../../..
>  include $(top_builddir)/src/Makefile.global
>
> +override CPPFLAGS := -I. $(CPPFLAGS)
> +
>  OBJS = \
>      condition_variable.o \
>      deadlock.o \
>      lmgr.o \
>      lock.o \
>      lwlock.o \
> -    lwlocknames.o \
>      predicate.o \
>      proc.o \
>      s_lock.o \
> diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl b/src/backend/storage/lmgr/generate-lwlocknames.pl
> index 7b93ecf6c1..a679a4ff54 100644
> --- a/src/backend/storage/lmgr/generate-lwlocknames.pl
> +++ b/src/backend/storage/lmgr/generate-lwlocknames.pl
> @@ -10,7 +10,6 @@ use Getopt::Long;
>  my $output_path = '.';
>
>  my $lastlockidx = -1;
> -my $continue = "\n";
>
>  GetOptions('outdir:s' => \$output_path);
>
> @@ -29,8 +28,6 @@ print $h $autogen;
>  print $h "/* there is deliberately not an #ifndef LWLOCKNAMES_H here */\n\n";
>  print $c $autogen, "\n";
>
> -print $c "const char *const IndividualLWLockNames[] = {";
> -
>  #
>  # First, record the predefined LWLocks listed in wait_event_names.txt.  We'll
>  # cross-check those with the ones in lwlocknames.txt.
> @@ -97,12 +94,10 @@ while (<$lwlocknames>)
>      while ($lastlockidx < $lockidx - 1)
>      {
>          ++$lastlockidx;
> -        printf $c "%s    \"<unassigned:%d>\"", $continue, $lastlockidx;
> -        $continue = ",\n";
> +        printf $c "[%s] = \"<unassigned:%d>\",\n", $lastlockidx, $lastlockidx;
>      }
> -    printf $c "%s    \"%s\"", $continue, $trimmedlockname;
> +    printf $c "[%s] = \"%s\",\n", $lockidx, $trimmedlockname;
>      $lastlockidx = $lockidx;
> -    $continue = ",\n";
>
>      print $h "#define $lockname (&MainLWLockArray[$lockidx].lock)\n";
>  }
> @@ -112,7 +107,6 @@ die
>    . "lwlocknames.txt"
>    if $i < scalar @wait_event_lwlocks;
>
> -printf $c "\n};\n";
>  print $h "\n";
>  printf $h "#define NUM_INDIVIDUAL_LWLOCKS        %s\n", $lastlockidx + 1;
>
> diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
> index 98fa6035cc..8aad9c6690 100644
> --- a/src/backend/storage/lmgr/lwlock.c
> +++ b/src/backend/storage/lmgr/lwlock.c
> @@ -115,8 +115,8 @@ StaticAssertDecl(LW_VAL_EXCLUSIVE > (uint32) MAX_BACKENDS,
>   * There are three sorts of LWLock "tranches":
>   *
>   * 1. The individually-named locks defined in lwlocknames.h each have their
> - * own tranche.  The names of these tranches appear in IndividualLWLockNames[]
> - * in lwlocknames.c.
> + * own tranche.  The names of these tranches come from lwlocknames.c into
> + * BuiltinTranchNames[] below.
>   *
>   * 2. There are some predefined tranches for built-in groups of locks.
>   * These are listed in enum BuiltinTrancheIds in lwlock.h, and their names
> @@ -129,9 +129,8 @@ StaticAssertDecl(LW_VAL_EXCLUSIVE > (uint32) MAX_BACKENDS,
>   * All these names are user-visible as wait event names, so choose with care
>   * ... and do not forget to update the documentation's list of wait events.
>   */
> -extern const char *const IndividualLWLockNames[];    /* in lwlocknames.c */
> -
>  static const char *const BuiltinTrancheNames[] = {
> +#include "lwlocknames.c"
>      [LWTRANCHE_XACT_BUFFER] = "XactBuffer",
>      [LWTRANCHE_COMMITTS_BUFFER] = "CommitTsBuffer",
>      [LWTRANCHE_SUBTRANS_BUFFER] = "SubtransBuffer",
> @@ -738,11 +737,7 @@ LWLockReportWaitEnd(void)
>  static const char *
>  GetLWTrancheName(uint16 trancheId)
>  {
> -    /* Individual LWLock? */
> -    if (trancheId < NUM_INDIVIDUAL_LWLOCKS)
> -        return IndividualLWLockNames[trancheId];
> -
> -    /* Built-in tranche? */
> +    /* Individual LWLock or built-in tranche? */
>      if (trancheId < LWTRANCHE_FIRST_USER_DEFINED)
>          return BuiltinTrancheNames[trancheId];
>
> diff --git a/src/backend/storage/lmgr/meson.build b/src/backend/storage/lmgr/meson.build
> index da32198f78..a12064ae8a 100644
> --- a/src/backend/storage/lmgr/meson.build
> +++ b/src/backend/storage/lmgr/meson.build
> @@ -5,11 +5,19 @@ backend_sources += files(
>    'deadlock.c',
>    'lmgr.c',
>    'lock.c',
> -  'lwlock.c',
>    'predicate.c',
>    'proc.c',
>    's_lock.c',
>    'spin.c',
>  )
>
> -generated_backend_sources += lwlocknames[1]
> +# this includes a .c file generated. Is there a better way?
> +lwlock_source = files('lwlock.c')

I don't understand this comment. Could you explain it a bit more?

> +
> +lwlock_lib = static_library('lwlock',
> +  lwlock_source,
> +  dependencies: [backend_code],
> +  include_directories: include_directories('../../../include/storage'),
> +  kwargs: internal_lib_args,
> +  )

Move the paren to the beginning of the line.

> +backend_link_with += lwlock_lib

Earlier in the thread you had said this:

> IMO it would be less ugly to have the origin file lwlocknames.txt be
> not a text file but a .h with a macro that can be defined by
> interested parties so that they can extract what they want from the
> file, like PG_CMDTAG or PG_KEYWORD.  Using Perl for this seems dirty...

I really like this idea, and would definitely be more inclined to see
a solution like this.

--
Tristan Partin
Neon (https://neon.tech)