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