Horton

Thread

Commits

  1. Rework lwlocknames.txt to become lwlocklist.h

  2. Improve notation of BuiltinTrancheNames

  1. make BuiltinTrancheNames less ugly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-01-23T10:25:25Z

    This array of tranche names is looking pretty ugly these days, and it'll
get worse as we add more members to it.  I propose to use C99 designated
initializers, like we've done for other arrays.  Patch attached.

The way I've coded in this patch, it means the array will now have 52
NULL pointers at the beginning.  I don't think this is a big deal and
makes the code prettier.  I see two alternatives:

1. Avoid all those NULLs by making each definition uglier (subtract
NUM_INDIVIDUAL_LWLOCKS from each array index) _and_ the usage of the
array by subtracting the same amount.  This saves 208 bytes at the
expense of making the code worse.

2. More invasively, rework generate-lwlocknames.pl so that both lwlocks
and these builtin tranche names appear in a single array.  (We could do
so by #include'ing lwlocknames.c at the top of the array).


Now, having written this proposal, I'm leaning towards idea 2 myself,
but since the patch here is less invasive, it seems worth having as
evidence.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them."  (Larry Wall)

  2. Re: make BuiltinTrancheNames less ugly

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-01-23T10:33:50Z

    On 23/01/2024 12:25, Alvaro Herrera wrote:
> This array of tranche names is looking pretty ugly these days, and it'll
> get worse as we add more members to it.  I propose to use C99 designated
> initializers, like we've done for other arrays.  Patch attached.
> 
> The way I've coded in this patch, it means the array will now have 52
> NULL pointers at the beginning.  I don't think this is a big deal and
> makes the code prettier.  I see two alternatives:
> 
> 1. Avoid all those NULLs by making each definition uglier (subtract
> NUM_INDIVIDUAL_LWLOCKS from each array index) _and_ the usage of the
> array by subtracting the same amount.  This saves 208 bytes at the
> expense of making the code worse.
> 
> 2. More invasively, rework generate-lwlocknames.pl so that both lwlocks
> and these builtin tranche names appear in a single array.  (We could do
> so by #include'ing lwlocknames.c at the top of the array).
> 
> 
> Now, having written this proposal, I'm leaning towards idea 2 myself,
> but since the patch here is less invasive, it seems worth having as
> evidence.

Idea 2 seems pretty straightforward, +1 for that.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  3. Re: make BuiltinTrancheNames less ugly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-01-23T11:47:42Z

    On 2024-Jan-23, Heikki Linnakangas wrote:

> On 23/01/2024 12:25, Alvaro Herrera wrote:

> > 2. More invasively, rework generate-lwlocknames.pl so that both lwlocks
> > and these builtin tranche names appear in a single array.  (We could do
> > so by #include'ing lwlocknames.c at the top of the array).

> Idea 2 seems pretty straightforward, +1 for that.

This is what I came up with.  For the compilation framework (both
Makefile and meson) I mostly just copied what we do in src/backend/node,
which seems to have passed CI just fine, but maybe there are better ways
to achieve it.

... oh, actually FreeBSD failed with this strange problem while linking:
[11:39:43.550] ld: error: undefined symbol: __dtraceenabled_postgresql___lwlock__wait__start
[11:39:43.551] >>> referenced by lwlock.c:1277 (../src/backend/storage/lmgr/lwlock.c:1277)
[11:39:43.551] >>>               lwlock.a.p/lwlock.c.o:(LWLockAcquire) in archive src/backend/storage/lmgr/lwlock.a
[11:39:43.551] >>> referenced by lwlock.c:1442 (../src/backend/storage/lmgr/lwlock.c:1442)
[11:39:43.551] >>>               lwlock.a.p/lwlock.c.o:(LWLockAcquireOrWait) in archive src/backend/storage/lmgr/lwlock.a
[11:39:43.551] >>> referenced by lwlock.c:1660 (../src/backend/storage/lmgr/lwlock.c:1660)
[11:39:43.551] >>>               lwlock.a.p/lwlock.c.o:(LWLockWaitForVar) in archive src/backend/storage/lmgr/lwlock.a
[11:39:43.551] 
  [ further similar lines ]
https://cirrus-ci.com/task/5055489315176448?logs=build#L1091

I have no idea what this means or how is it related to what I'm doing.


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 ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"In Europe they call me Niklaus Wirth; in the US they call me Nickel's worth.
 That's because in Europe they call me by name, and in the US by value!"

  4. Re: make BuiltinTrancheNames less ugly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-01-23T12:39:58Z

    On 2024-Jan-23, Alvaro Herrera wrote:

> ... oh, actually FreeBSD failed with this strange problem while linking:
> [11:39:43.550] ld: error: undefined symbol: __dtraceenabled_postgresql___lwlock__wait__start
> [11:39:43.551] >>> referenced by lwlock.c:1277 (../src/backend/storage/lmgr/lwlock.c:1277)
> [11:39:43.551] >>>               lwlock.a.p/lwlock.c.o:(LWLockAcquire) in archive src/backend/storage/lmgr/lwlock.a
> [11:39:43.551] >>> referenced by lwlock.c:1442 (../src/backend/storage/lmgr/lwlock.c:1442)
> [11:39:43.551] >>>               lwlock.a.p/lwlock.c.o:(LWLockAcquireOrWait) in archive src/backend/storage/lmgr/lwlock.a
> [11:39:43.551] >>> referenced by lwlock.c:1660 (../src/backend/storage/lmgr/lwlock.c:1660)
> [11:39:43.551] >>>               lwlock.a.p/lwlock.c.o:(LWLockWaitForVar) in archive src/backend/storage/lmgr/lwlock.a
> [11:39:43.551] 

So what's going on here is that lwlock.c is being compiled to a separate
archive (lwlock.a), and when linking it wants to see the dtrace symbols
(__dtraceenabled_postgresql___lwlock__wait__start et al) but it looks
like they're not defined.

I installed systemtap on my machine and reconfigured meson with it so
that it uses dtrace, and it compiles successfully.  One interesting
point: on my ninja log, I see dtrace being invoked, which I do not in
the FreeBSD log.

I continue to not understand what is going on.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"El sabio habla porque tiene algo que decir;
el tonto, porque tiene que decir algo" (Platon).

  5. Re: make BuiltinTrancheNames less ugly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-01-23T13:05:13Z

    On 2024-Jan-23, Alvaro Herrera wrote:

> This is what I came up with.

Hm, I forgot the attachment, thanks Heikki for the reminder.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Las mujeres son como hondas:  mientras más resistencia tienen,
 más lejos puedes llegar con ellas"  (Jonas Nightingale, Leap of Faith)

  6. Re: make BuiltinTrancheNames less ugly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-01-23T14:42:39Z

    On 2024-Jan-23, Alvaro Herrera wrote:

> ... oh, actually FreeBSD failed with this strange problem while linking:

I figured this out.  For dtrace support, we need to run dtrace on all
the objects produced by the compilation step, but because I took
lwlock.o out of the objects used to produce the backend into its own
"library", it needs to be included explicitly.  (I think this is not a
problem for things like nodes/copyfuncs.c only because those files don't
have any use of TRACE macros).

So here's v3, which now passes fully in CI.

I'm a total newbie to Meson, so it's likely that there are better ways
to implement this.  I'll leave this here for a little bit in case
anybody wants to comment.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"El sudor es la mejor cura para un pensamiento enfermo" (Bardia)

  7. Re: make BuiltinTrancheNames less ugly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-01-24T14:09:49Z

    On 2024-Jan-23, Alvaro Herrera wrote:

> I'm a total newbie to Meson, so it's likely that there are better ways
> to implement this.  I'll leave this here for a little bit in case
> anybody wants to comment.

OK, I pushed the array definition, and here's the other bits as a
followup patch.  I'll add it to the next commitfest, though I hope to
get it committed before then, either in this form or whatever different
Meson trickery is recommended.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Every machine is a smoke machine if you operate it wrong enough."
https://twitter.com/libseybieda/status/1541673325781196801

  8. Re: make BuiltinTrancheNames less ugly

    Tristan Partin <tristan@neon.tech> — 2024-02-12T17:01:51Z

    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)

  9. Re: make BuiltinTrancheNames less ugly

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-02-23T09:31:12Z

    On 12/02/2024 19:01, Tristan Partin wrote:
> On Wed Jan 24, 2024 at 8:09 AM CST, Alvaro Herrera wrote:
>> 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.

+1 to that idea from me too. Seems pretty straightforward.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  10. Re: make BuiltinTrancheNames less ugly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-03-01T14:00:49Z

    On 2024-Feb-23, Heikki Linnakangas wrote:

> On 12/02/2024 19:01, Tristan Partin wrote:
> > On Wed Jan 24, 2024 at 8:09 AM CST, Alvaro Herrera wrote:
> > > 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.
> 
> +1 to that idea from me too. Seems pretty straightforward.

OK, here's a patch that does it.  I have not touched Meson yet.

I'm pretty disappointed of not being able to remove
generate-lwlocknames.pl (it now generates the .h, no longer the .c
file), but I can't find a way to do the equivalent #defines in CPP ...
it'd require invoking the C preprocessor twice.  Maybe an intermediate
.h file would solve the problem, but I have no idea how would that work
with Meson.  I guess I'll do it in Make and let somebody suggest a Meson
way.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)

  11. Re: make BuiltinTrancheNames less ugly

    Tristan Partin <tristan@neon.tech> — 2024-03-01T16:42:19Z

    On Fri Mar 1, 2024 at 8:00 AM CST, Alvaro Herrera wrote:
> On 2024-Feb-23, Heikki Linnakangas wrote:
>
> > On 12/02/2024 19:01, Tristan Partin wrote:
> > > On Wed Jan 24, 2024 at 8:09 AM CST, Alvaro Herrera wrote:
> > > > 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.
> > 
> > +1 to that idea from me too. Seems pretty straightforward.
>
> OK, here's a patch that does it.  I have not touched Meson yet.
>
> I'm pretty disappointed of not being able to remove
> generate-lwlocknames.pl (it now generates the .h, no longer the .c
> file), but I can't find a way to do the equivalent #defines in CPP ...
> it'd require invoking the C preprocessor twice.  Maybe an intermediate
> .h file would solve the problem, but I have no idea how would that work
> with Meson.  I guess I'll do it in Make and let somebody suggest a Meson
> way.

I can help you with Meson if you get the Make implementation done.

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

  12. Re: make BuiltinTrancheNames less ugly

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-03-11T12:01:52Z

    On 2024-Mar-01, Tristan Partin wrote:

> On Fri Mar 1, 2024 at 8:00 AM CST, Alvaro Herrera wrote:

> > I'm pretty disappointed of not being able to remove
> > generate-lwlocknames.pl (it now generates the .h, no longer the .c
> > file), but I can't find a way to do the equivalent #defines in CPP ...
> > it'd require invoking the C preprocessor twice.  Maybe an intermediate
> > .h file would solve the problem, but I have no idea how would that work
> > with Meson.  I guess I'll do it in Make and let somebody suggest a Meson
> > way.
> 
> I can help you with Meson if you get the Make implementation done.

Actually I realized that we need to keep the Perl script anyway because
we want to be able to cross-check the wait_event_names.txt files to
ensure we generate the correct documentation.  Since it was simple
enough, I added the Meson support already.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Essentially, you're proposing Kevlar shoes as a solution for the problem
that you want to walk around carrying a loaded gun aimed at your foot.
(Tom Lane)