Обсуждение: make BuiltinTrancheNames less ugly
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)
Вложения
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)
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!"
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).
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)
Вложения
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)
Вложения
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
Вложения
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)
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)
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)
Вложения
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)
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)