Обсуждение: Verify predefined LWLocks tranches have entries in wait_event_names.txt

Hi hackers,

Some discrepancies between wait_event_names.txt and predefined LWLocks tranches
have been observed, see: a493e741d32 ,08b9b9e043b and c3623703f36.

To prevent new discrepancies from occurring, this patch series aims to $SUBJECT
(as 5b1b9bce844 did for predefined LWLocks).

The patch series contains:

0001 - Extracts the predefined LWLocks tranches from lwlock.c and puts them
in a new file (namely lwlocktranchelist.h). This way, we can include this file
in lwlock.c using the same macro pattern as we do for lwlocklist.h.

This gives us the chance to cross-check with wait_event_names.txt in
generate-lwlocknames.pl in the same way as 5b1b9bce844 did.

It's much simpler and makes more sense than having generate-lwlocknames.pl
parsing lwlock.c.

0002 - While doing changes in generate-lwlocknames.pl, I noticed that 
$continue is not used (Oversight in commit da952b415f4), so let's remove it.

0003 - Cross-check lists of predefined tranches

Same idea as 5b1b9bce844 i.e if the lists (in wait_event_names.txt and
lwlocktranchelist.h) do not match exactly, building will fail.

I wonder if once these get in, we could also generate "BuiltinTrancheIds" (in
lwlock.h) from lwlocktranchelist.h with generate-lwlocknames.pl.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:
> 0002 - While doing changes in generate-lwlocknames.pl, I noticed that 
> $continue is not used (Oversight in commit da952b415f4), so let's remove it.

I haven't looked closely at the other patches, but I went ahead and
committed this one to get it out of the way.

-- 
nathan

On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:
> +#define PG_BUILTIN_LWTRANCHE(id, name) [id] = name,
> +#include "storage/lwlocktranchelist.h"
> +#undef PG_BUILTIN_LWTRANCHE

Why not reuse PG_LWLOCK for this?

> +    # Stop recording if we reach another section.
> +    last if /^Section:/;

Can we add a note to wait_event_names.txt about the required
ordering/matching of the non-predefined LWLocks?  Otherwise, these patches
look pretty good to me.

-- 
nathan

On Mon, Jul 21, 2025 at 03:20:55PM -0500, Nathan Bossart wrote:
> On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:
>> +#define PG_BUILTIN_LWTRANCHE(id, name) [id] = name,
>> +#include "storage/lwlocktranchelist.h"
>> +#undef PG_BUILTIN_LWTRANCHE
> 
> Why not reuse PG_LWLOCK for this?
> 
>> +    # Stop recording if we reach another section.
>> +    last if /^Section:/;
> 
> Can we add a note to wait_event_names.txt about the required
> ordering/matching of the non-predefined LWLocks?  Otherwise, these patches
> look pretty good to me.

Something else I just thought of: could we remove the list of built-in
tranches in lwlock.h with some macro magic that generates it from
lwlocktranchelist.h, too?

-- 
nathan

On Mon, Jul 21, 2025 at 03:28:14PM -0500, Nathan Bossart wrote:
> On Mon, Jul 21, 2025 at 03:20:55PM -0500, Nathan Bossart wrote:
>> Can we add a note to wait_event_names.txt about the required
>> ordering/matching of the non-predefined LWLocks?  Otherwise, these patches
>> look pretty good to me.
>
> Something else I just thought of: could we remove the list of built-in
> tranches in lwlock.h with some macro magic that generates it from
> lwlocktranchelist.h, too?

Ah, you mean removing the need to have to maintain BuiltinTrancheIds.
This structure depends on NUM_INDIVIDUAL_LWLOCKS for the start value.
Not really an objection per-se, but trying to automate everything may
impact the readability of this area of the code.
--
Michael

On Tue, Jul 22, 2025 at 08:02:52AM +0900, Michael Paquier wrote:
> Ah, you mean removing the need to have to maintain BuiltinTrancheIds.
> This structure depends on NUM_INDIVIDUAL_LWLOCKS for the start value.
> Not really an objection per-se, but trying to automate everything may
> impact the readability of this area of the code.

I bet we could maintain a decent level of readability with some extra
commentary.  IMHO it's worth it to avoid maintaining duplicate lists.  But
that's not something I feel terribly strong about, if others disagree.
FWIW I was imagining something like this:

    typedef enum BuiltinTrancheIds
    {
        LWTRANCHE_INVALID = NUM_INDIVIDUAL_LWLOCKS - 1,
    #define PG_BUILTIN_LWTRANCHE(id, name) id,
    #include "storage/lwlocktranchelist.h"
    #undef PG_BUILTIN_LWTRANCHE
        LWTRANCHE_FIRST_USER_DEFINED,
    }           BuiltinTrancheIds;

-- 
nathan

On Mon, Jul 21, 2025 at 08:34:41PM -0500, Nathan Bossart wrote:
> I bet we could maintain a decent level of readability with some extra
> commentary.  IMHO it's worth it to avoid maintaining duplicate lists.  But
> that's not something I feel terribly strong about, if others disagree.
> FWIW I was imagining something like this:
>
>     typedef enum BuiltinTrancheIds
>     {
>         LWTRANCHE_INVALID = NUM_INDIVIDUAL_LWLOCKS - 1,

Something like that would be OK for me.
--
Michael

Hi,

On Mon, Jul 21, 2025 at 03:28:14PM -0500, Nathan Bossart wrote:
> On Mon, Jul 21, 2025 at 03:20:55PM -0500, Nathan Bossart wrote:
> > On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:
> >> +#define PG_BUILTIN_LWTRANCHE(id, name) [id] = name,
> >> +#include "storage/lwlocktranchelist.h"
> >> +#undef PG_BUILTIN_LWTRANCHE
> > 
> > Why not reuse PG_LWLOCK for this?
> > 
> >> +    # Stop recording if we reach another section.
> >> +    last if /^Section:/;
> > 
> > Can we add a note to wait_event_names.txt about the required
> > ordering/matching of the non-predefined LWLocks?  Otherwise, these patches
> > look pretty good to me.
> 
> Something else I just thought of: could we remove the list of built-in
> tranches in lwlock.h with some macro magic that generates it from
> lwlocktranchelist.h, too?

Thanks for looking at the patch series!

Yeah, I also had in mind to auto-generate this enum list (see last sentence
in the initial email [1]) but did not think that much on how to do it and was
waiting for this patch series to go in before looking at it.

But as I like your macro idea and as you mentioned it here too, then let's do
it now: it's done in 0003 attached.

[1]: https://www.postgresql.org/message-id/aHpOgwuFQfcFMZ/B%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Mon, Jul 21, 2025 at 03:20:55PM -0500, Nathan Bossart wrote:
> On Fri, Jul 18, 2025 at 01:39:15PM +0000, Bertrand Drouvot wrote:
> > +#define PG_BUILTIN_LWTRANCHE(id, name) [id] = name,
> > +#include "storage/lwlocktranchelist.h"
> > +#undef PG_BUILTIN_LWTRANCHE
> 
> Why not reuse PG_LWLOCK for this?

I was not sure we'd want add a new file or add those in lwlocklist.h (in which
case 2 macros would have been simpler). As it looks like we are going with a
new file then yeah we can reuse PG_LWLOCK: done in v2 just shared up-thread.

> > +    # Stop recording if we reach another section.
> > +    last if /^Section:/;
> 
> Can we add a note to wait_event_names.txt about the required
> ordering/matching of the non-predefined LWLocks? 

Sure, done in v2 too.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Tue, Jul 22, 2025 at 06:23:50AM +0000, Bertrand Drouvot wrote:
> Sure, done in v2 too.

I stared at this patch some more and came up with the attached.  The
biggest change is that I've moved the list of built-in LWLock tranches to
the existing lwlocklist.h file.  That simplifies the patch and centralizes
these lists.  This is arguably a bit too much preprocessor magic, though.
Thoughts?

-- 
nathan

On Tue, Jul 22, 2025 at 02:25:13PM -0500, Nathan Bossart wrote:
> I stared at this patch some more and came up with the attached.  The
> biggest change is that I've moved the list of built-in LWLock tranches to
> the existing lwlocklist.h file.  That simplifies the patch and centralizes
> these lists.  This is arguably a bit too much preprocessor magic, though.
> Thoughts?

With the argument about checking the consistency of the data between
wait_event_names.txt, that looks like an improvement to me.  And as
far as I can see, the check you are adding in generate-lwlocknames.pl
also makes sure that the ordering of the entries is correct.

In short, I side with the argument that this extra magic will save
cycles overall.
--
Michael

Hi,

On Wed, Jul 23, 2025 at 12:23:37PM +0900, Michael Paquier wrote:
> On Tue, Jul 22, 2025 at 02:25:13PM -0500, Nathan Bossart wrote:
> > I stared at this patch some more and came up with the attached.  The
> > biggest change is that I've moved the list of built-in LWLock tranches to
> > the existing lwlocklist.h file.  That simplifies the patch and centralizes
> > these lists.  This is arguably a bit too much preprocessor magic, though.

Yeah, but OTOH we avoid the addition of a new file and we avoid changing some
meson and make files. Also the distinction is still clear as we are using a new
macro name. So that's fine by me.

> > Thoughts?

I noticed that you removed the LWTRANCHE_ prefix in lwlocklist.h, while I agree
that's not needed, that could be misleading for people that grep for things like
"LWTRANCHE_XACT_BUFFER" for example. I don't think that's a big deal though.

Also, I think we don't need to read lwlocklist.h twice in generate-lwlocknames.pl.

PFA v4 where the only change compared to v3 is that it reads lwlocklist.h once
in generate-lwlocknames.pl.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Jul 23, 2025 at 06:22:16AM +0000, Bertrand Drouvot wrote:
> PFA v4 where the only change compared to v3 is that it reads lwlocklist.h once
> in generate-lwlocknames.pl.

Committed.

-- 
nathan