Re: verify predefined LWLocks have entries in wait_event_names.txt

Hi,

On Fri, Jan 05, 2024 at 11:46:20AM -0600, Nathan Bossart wrote:
> On Fri, Jan 05, 2024 at 10:42:03AM -0600, Nathan Bossart wrote:
> > On Fri, Jan 05, 2024 at 07:39:39AM +0000, Bertrand Drouvot wrote:
> >> + die "lists of predefined LWLocks in lwlocknames.txt and wait_event_names.txt do not match"
> >> + unless $wait_event_lwlocks[$i] eq $lockname;
> >>
> >> What about printing $wait_event_lwlocks[$i] and $lockname in the error message?
> >> Something like?
> >>
> >> "
> >> die "lists of predefined LWLocks in lwlocknames.txt and wait_event_names.txt do not match (comparing $lockname and $wait_event_lwlocks[$i])"
> >> unless $wait_event_lwlocks[$i] eq $lockname;
> >> "
> >>
> >> I think that would give more clues for debugging purpose.
> >
> > Sure, I'll add something like that. I think this particular scenario is
> > less likely, but that's not a reason to make the error message hard to
> > decipher.
>
> Here is a new version of the patch with this change.

Thanks!

> I also tried to make the verification logic less fragile. Instead of
> depending on the exact location of empty lines in wait_event_names.txt, v3
> adds a marker comment below the list that clearly indicates it should not
> be changed. This simplifies the verification code a bit, too.

Yeah, good idea, I think that's easier to read.

Sorry, I missed this in my first review, but instead of:

- input: files('../../backend/storage/lmgr/lwlocknames.txt'),
+ input: [files('../../backend/storage/lmgr/lwlocknames.txt'), files('../../backend/utils/activity/wait_event_names.txt')],

what about?

input: files(
'../../backend/storage/lmgr/lwlocknames.txt',
'../../backend/utils/activity/wait_event_names.txt',
),

It's done that way in doc/src/sgml/meson.build for example.

Except for the above, the patch looks good to me.

Regards,

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