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

(new thread)

On Tue, Jan 02, 2024 at 10:34:11AM -0500, Robert Haas wrote:
> On Wed, Dec 27, 2023 at 10:36 AM Nathan Bossart
> <nathandbossart@gmail.com> wrote:
>> Thanks!  I also noticed that WALSummarizerLock probably needs a mention in
>> wait_event_names.txt.
> 
> Fixed.

I think we're supposed to omit the "Lock" suffix in wait_event_names.txt.

> It seems like it would be good if there were an automated cross-check
> between lwlocknames.txt and wait_event_names.txt.

+1.  Here's a hastily-thrown-together patch for that.  I basically copied
003_check_guc.pl and adjusted it for this purpose.  This test only checks
that everything in lwlocknames.txt has a matching entry in
wait_event_names.txt.  It doesn't check that everything in the predefined
LWLock section of wait_event_names.txt has an entry in lwlocknames.txt.
AFAICT that would be a little more difficult because you can't distinguish
between the two in pg_wait_events.

Even with this test, I worry that we could easily forget to add entries in
wait_event_names.txt for the non-predefined locks, but I don't presently
have a proposal for how to prevent that.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 2, 2024 at 12:31 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I think we're supposed to omit the "Lock" suffix in wait_event_names.txt.

Ugh, sorry. But also, why in the world?

> > It seems like it would be good if there were an automated cross-check
> > between lwlocknames.txt and wait_event_names.txt.
>
> +1.  Here's a hastily-thrown-together patch for that.  I basically copied
> 003_check_guc.pl and adjusted it for this purpose.  This test only checks
> that everything in lwlocknames.txt has a matching entry in
> wait_event_names.txt.  It doesn't check that everything in the predefined
> LWLock section of wait_event_names.txt has an entry in lwlocknames.txt.
> AFAICT that would be a little more difficult because you can't distinguish
> between the two in pg_wait_events.
>
> Even with this test, I worry that we could easily forget to add entries in
> wait_event_names.txt for the non-predefined locks, but I don't presently
> have a proposal for how to prevent that.

It certainly seems better to check what we can than to check nothing.

Suggestions:

- Check in both directions instead of just one?

- Verify ordering?

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Jan 02, 2024 at 01:13:16PM -0500, Robert Haas wrote:
> On Tue, Jan 2, 2024 at 12:31 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I think we're supposed to omit the "Lock" suffix in wait_event_names.txt.
> 
> Ugh, sorry. But also, why in the world?

That seems to date back to commit 14a9101.  I can agree that the suffix is
somewhat redundant since these are already marked as type "LWLock", but
I'll admit I've been surprised by this before, too.  IMHO it makes this
proposed test more important because you can't just grep for a different
lock to find all the places you need to update.

> - Check in both directions instead of just one?
> 
> - Verify ordering?

To do those things, I'd probably move the test to one of the scripts that
generates the documentation or header file (pg_wait_events doesn't tell us
whether a lock is predefined or what order it's listed in).  That'd cause
failures at build time instead of during testing, which might be kind of
nice, too.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 02, 2024 at 11:31:20AM -0600, Nathan Bossart wrote:
> +# Find the location of lwlocknames.h.
> +my $include_dir = $node->config_data('--includedir');
> +my $lwlocknames_file = "$include_dir/server/storage/lwlocknames.h";

I am afraid that this is incorrect because an installation could
decide to install server-side headers in a different path than
$include/server/.  Using --includedir-server would be the correct
answer, appending "storage/lwlocknames.h" to the path retrieved from
pg_config.
--
Michael

On Tue, Jan 2, 2024 at 4:45 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> That seems to date back to commit 14a9101.  I can agree that the suffix is
> somewhat redundant since these are already marked as type "LWLock", but
> I'll admit I've been surprised by this before, too.  IMHO it makes this
> proposed test more important because you can't just grep for a different
> lock to find all the places you need to update.

I agree. I am pretty sure that the reason this happened in the first
place is that I grepped for the name of some other LWLock and adjusted
things for the new lock at every place where that found a hit.

> > - Check in both directions instead of just one?
> >
> > - Verify ordering?
>
> To do those things, I'd probably move the test to one of the scripts that
> generates the documentation or header file (pg_wait_events doesn't tell us
> whether a lock is predefined or what order it's listed in).  That'd cause
> failures at build time instead of during testing, which might be kind of
> nice, too.

Yeah, I think that would be better.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On Tue, Jan 02, 2024 at 10:49:03PM -0500, Robert Haas wrote:
> On Tue, Jan 2, 2024 at 4:45 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > That seems to date back to commit 14a9101.  I can agree that the suffix is
> > somewhat redundant since these are already marked as type "LWLock", but
> > I'll admit I've been surprised by this before, too.  IMHO it makes this
> > proposed test more important because you can't just grep for a different
> > lock to find all the places you need to update.
> 
> I agree. I am pretty sure that the reason this happened in the first
> place is that I grepped for the name of some other LWLock and adjusted
> things for the new lock at every place where that found a hit.
> 
> > > - Check in both directions instead of just one?
> > >
> > > - Verify ordering?
> >
> > To do those things, I'd probably move the test to one of the scripts that
> > generates the documentation or header file (pg_wait_events doesn't tell us
> > whether a lock is predefined or what order it's listed in).  That'd cause
> > failures at build time instead of during testing, which might be kind of
> > nice, too.
> 
> Yeah, I think that would be better.

+1 to add a test and put in a place that would produce failures at build time.
I think that having the test in the script that generates the header file is more
appropriate (as building the documentation looks less usual to me when working on
a patch).

Regards,

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

On Wed, Jan 03, 2024 at 07:59:45AM +0000, Bertrand Drouvot wrote:
> +1 to add a test and put in a place that would produce failures at build time.
> I think that having the test in the script that generates the header file is more
> appropriate (as building the documentation looks less usual to me when working on
> a patch).

Okay, I did that in v2.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Jan 03, 2024 at 11:34:25AM +0900, Michael Paquier wrote:
> On Tue, Jan 02, 2024 at 11:31:20AM -0600, Nathan Bossart wrote:
>> +# Find the location of lwlocknames.h.
>> +my $include_dir = $node->config_data('--includedir');
>> +my $lwlocknames_file = "$include_dir/server/storage/lwlocknames.h";
> 
> I am afraid that this is incorrect because an installation could
> decide to install server-side headers in a different path than
> $include/server/.  Using --includedir-server would be the correct
> answer, appending "storage/lwlocknames.h" to the path retrieved from
> pg_config.

Ah, good to know, thanks.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Hi,

On Fri, Jan 05, 2024 at 12:11:44AM -0600, Nathan Bossart wrote:
> On Wed, Jan 03, 2024 at 07:59:45AM +0000, Bertrand Drouvot wrote:
> > +1 to add a test and put in a place that would produce failures at build time.
> > I think that having the test in the script that generates the header file is more
> > appropriate (as building the documentation looks less usual to me when working on
> > a patch).
> 
> Okay, I did that in v2.

Thanks!

> +# NB: Predefined locks (those declared in lwlocknames.txt) must be listed in
> +# the top section of locks and must be listed in the same order as in
> +# lwlocknames.txt.
> +#
>  
>  Section: ClassName - WaitEventLWLock
>  
> @@ -326,6 +330,12 @@ NotifyQueueTail    "Waiting to update limit on <command>NOTIFY</command> message st
>  WaitEventExtension    "Waiting to read or update custom wait events information for extensions."
>  WALSummarizer    "Waiting to read or update WAL summarization state."
>  
> +#
> +# Predefined LWLocks (those declared in lwlocknames.txt) must be listed in the
> +# section above and must be listed in the same order as in lwlocknames.txt.
> +# Other LWLocks must be listed in the section below.
> +#
> +

Another option could be to create a sub-section for predefined LWLocks that are
part of lwlocknames.txt and then sort both list (the one in the sub-section and
the one in lwlocknames.txt). That would avoid the "must be listed in the same order"
constraint. That said, I think the way it is done in the patch is fine because
if one does not follow the constraint then the build would fail.

I did a few tests leading to:

CommitBDTTsSLRALock defined in lwlocknames.txt but missing from wait_event_names.txt at ./generate-lwlocknames.pl line
107,<$lwlocknames> line 58.
 

OR 

lists of predefined LWLocks in lwlocknames.txt and wait_event_names.txt do not match at ./generate-lwlocknames.pl line
109,<$lwlocknames> line 46.
 

OR

CommitBDTTsSLRALock defined in wait_event_names.txt but missing from lwlocknames.txt at ./generate-lwlocknames.pl line
126,<$lwlocknames> line 57.
 

So, that looks good to me except one remark regarding:

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

Regards,

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

Thanks for reviewing.

On Fri, Jan 05, 2024 at 07:39:39AM +0000, Bertrand Drouvot wrote:
> Another option could be to create a sub-section for predefined LWLocks that are
> part of lwlocknames.txt and then sort both list (the one in the sub-section and
> the one in lwlocknames.txt). That would avoid the "must be listed in the same order"
> constraint. That said, I think the way it is done in the patch is fine because
> if one does not follow the constraint then the build would fail.

IMHO the ordering constraint makes it easier for humans to verify the lists
match.

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

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

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.

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.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

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

On Sat, Jan 06, 2024 at 09:03:52AM +0000, Bertrand Drouvot wrote:
> 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.

I fixed this in v4.

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

Thanks for reviewing!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Hi,

On Sat, Jan 06, 2024 at 10:18:52AM -0600, Nathan Bossart wrote:
> On Sat, Jan 06, 2024 at 09:03:52AM +0000, Bertrand Drouvot wrote:
> > 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.
> 
> I fixed this in v4.

Thanks!

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

I think the "[" and "]" are not needed here.

Regards,

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

On Mon, Jan 08, 2024 at 07:59:10AM +0000, Bertrand Drouvot wrote:
> +  input: [files(
> +    '../../backend/storage/lmgr/lwlocknames.txt',
> +    '../../backend/utils/activity/wait_event_names.txt')],
> 
> I think the "[" and "]" are not needed here.

D'oh!  Fixed in v5.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Sorry for the noise.  I spent some more time tidying this up for commit,
which I am hoping to do in the next day or two.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Hi,

On Mon, Jan 08, 2024 at 04:02:12PM -0600, Nathan Bossart wrote:
> Sorry for the noise.  I spent some more time tidying this up for commit,
> which I am hoping to do in the next day or two.

Thanks! v6 looks good to me.

Regards,

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

On Tue, Jan 09, 2024 at 04:55:07AM +0000, Bertrand Drouvot wrote:
> Thanks! v6 looks good to me.

WFM.  Thanks for putting in place this sanity check when compiling.
--
Michael

On Tue, Jan 09, 2024 at 02:26:20PM +0900, Michael Paquier wrote:
> On Tue, Jan 09, 2024 at 04:55:07AM +0000, Bertrand Drouvot wrote:
>> Thanks! v6 looks good to me.
> 
> WFM.  Thanks for putting in place this sanity check when compiling.

Committed.  Thanks for reviewing!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com