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
Attachments:
v1-0001-verify-wait-events-for-all-lwlocks.patchtext/x-diff; charset=us-asciiDownload
From 5031b55d8e1095ca919fe9a088ec0539b28984fc Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 2 Jan 2024 11:19:20 -0600
Subject: [PATCH v1 1/1] verify wait events for all lwlocks
---
.../utils/activity/wait_event_names.txt | 2 +-
src/test/modules/test_misc/meson.build | 1 +
.../modules/test_misc/t/005_check_lwlock.pl | 51 +++++++++++++++++++
3 files changed, 53 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/test_misc/t/005_check_lwlock.pl
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index d876f8a667..f61ec3e59d 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -324,7 +324,7 @@ XactTruncation "Waiting to execute <function>pg_xact_status</function> or update
WrapLimitsVacuum "Waiting to update limits on transaction id and multixact consumption."
NotifyQueueTail "Waiting to update limit on <command>NOTIFY</command> message storage."
WaitEventExtension "Waiting to read or update custom wait events information for extensions."
-WALSummarizerLock "Waiting to read or update WAL summarization state."
+WALSummarizer "Waiting to read or update WAL summarization state."
XactBuffer "Waiting for I/O on a transaction status SLRU buffer."
CommitTsBuffer "Waiting for I/O on a commit timestamp SLRU buffer."
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index 911084ac0f..13bc6cb423 100644
--- a/src/test/modules/test_misc/meson.build
+++ b/src/test/modules/test_misc/meson.build
@@ -10,6 +10,7 @@ tests += {
't/002_tablespace.pl',
't/003_check_guc.pl',
't/004_io_direct.pl',
+ 't/005_check_lwlock.pl',
],
},
}
diff --git a/src/test/modules/test_misc/t/005_check_lwlock.pl b/src/test/modules/test_misc/t/005_check_lwlock.pl
new file mode 100644
index 0000000000..aff83e879e
--- /dev/null
+++ b/src/test/modules/test_misc/t/005_check_lwlock.pl
@@ -0,0 +1,51 @@
+# Tests to cross-check the consistency of pg_wait_events with lwlocknames.h.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->start;
+
+# Grab the names of all the LWLocks from pg_wait_events.
+my $all_lwlocks = $node->safe_psql(
+ 'postgres',
+ "SELECT name FROM pg_wait_events WHERE type = 'LWLock'");
+my @all_lwlocks_array = split("\n", $all_lwlocks);
+
+# Find the location of lwlocknames.h.
+my $include_dir = $node->config_data('--includedir');
+my $lwlocknames_file = "$include_dir/server/storage/lwlocknames.h";
+
+# Read the list of locks from lwlocknames.h.
+my $num_tests = 0;
+my @lwlocks_found;
+open(my $contents, '<', $lwlocknames_file)
+ || die "Could not open $lwlocknames_file: $!";
+while (my $line = <$contents>)
+{
+ if ($line =~ m/^#define ([a-zA-Z]+)Lock .*/)
+ {
+ push @lwlocks_found, $1;
+ }
+}
+close $contents;
+
+# Cross-check that all the locks found in lwlocknames.h have matching entries
+# in pg_wait_events.
+my %all_lwlocks_hash = map { $_ => 1 } @all_lwlocks_array;
+my @missing_from_wait_events = grep(!$all_lwlocks_hash{$_}, @lwlocks_found);
+is(scalar(@missing_from_wait_events),
+ 0, "no LWLocks missing from pg_wait_events");
+
+foreach my $lwlock (@missing_from_wait_events)
+{
+ print(
+ "found $lwlock in lwlocknames.h, missing from wait_event_names.txt\n"
+ );
+}
+
+done_testing();
--
2.25.1
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
Attachments:
v2-0001-verify-lists-of-predefined-LWLocks-in-lwlocknames.patchtext/x-diff; charset=us-asciiDownload
From 587f6a2c108fc7496fcb3d5764ca06ac5fb21326 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 5 Jan 2024 00:07:40 -0600
Subject: [PATCH v2 1/1] verify lists of predefined LWLocks in lwlocknames.txt
and wait_event_names.txt match
---
src/backend/Makefile | 2 +-
src/backend/storage/lmgr/Makefile | 4 +-
.../storage/lmgr/generate-lwlocknames.pl | 63 +++++++++++++++++++
.../utils/activity/wait_event_names.txt | 10 +++
src/include/storage/meson.build | 2 +-
5 files changed, 77 insertions(+), 4 deletions(-)
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 816461aa17..7d2ea7d54a 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -130,7 +130,7 @@ $(top_builddir)/src/port/libpgport_srv.a: | submake-libpgport
parser/gram.h: parser/gram.y
$(MAKE) -C parser gram.h
-storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt
+storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt utils/activity/wait_event_names.txt
$(MAKE) -C storage/lmgr lwlocknames.h lwlocknames.c
utils/activity/wait_event_types.h: utils/activity/generate-wait_event_types.pl utils/activity/wait_event_names.txt
diff --git a/src/backend/storage/lmgr/Makefile b/src/backend/storage/lmgr/Makefile
index c48ba943c4..504480e847 100644
--- a/src/backend/storage/lmgr/Makefile
+++ b/src/backend/storage/lmgr/Makefile
@@ -39,8 +39,8 @@ s_lock_test: s_lock.c $(top_builddir)/src/common/libpgcommon.a $(top_builddir)/s
lwlocknames.c: lwlocknames.h
touch $@
-lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt generate-lwlocknames.pl
- $(PERL) $(srcdir)/generate-lwlocknames.pl $<
+lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt $(top_srcdir)/src/backend/utils/activity/wait_event_names.txt generate-lwlocknames.pl
+ $(PERL) $(srcdir)/generate-lwlocknames.pl $^
check: s_lock_test
./s_lock_test
diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl b/src/backend/storage/lmgr/generate-lwlocknames.pl
index bef5e16e11..9afee7984b 100644
--- a/src/backend/storage/lmgr/generate-lwlocknames.pl
+++ b/src/backend/storage/lmgr/generate-lwlocknames.pl
@@ -15,6 +15,7 @@ my $continue = "\n";
GetOptions('outdir:s' => \$output_path);
open my $lwlocknames, '<', $ARGV[0] or die;
+open my $wait_event_names, '<', $ARGV[1] or die;
# Include PID in suffix in case parallel make runs this multiple times.
my $htmp = "$output_path/lwlocknames.h.tmp$$";
@@ -30,6 +31,59 @@ 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.
+#
+my @wait_event_lwlocks;
+my $found_lwlock_section = 0;
+my $recording_lwlocks = 0;
+
+while (<$wait_event_names>)
+{
+ chomp;
+
+ # Skip comments
+ next if /^#/;
+
+ # Skip empty lines
+ if (/^\s*$/)
+ {
+ #
+ # If we encounter an empty line while recording the LWLocks listed in
+ # wait_event_names.txt, we are done.
+ #
+ last if $recording_lwlocks;
+
+ #
+ # Begin recording the LWLocks listed in wait_event_names.txt following
+ # the empty line after the WaitEventLWLock section declaration.
+ #
+ $recording_lwlocks = 1 if $found_lwlock_section;
+ next;
+ }
+
+ #
+ # Prepare to record the LWLocks when we find the WaitEventLWLock section
+ # declaration.
+ #
+ if (/^Section: ClassName(.*)/)
+ {
+ my $section_name = $_;
+ $section_name =~ s/^.*- //;
+ $found_lwlock_section = 1 if $section_name eq "WaitEventLWLock";
+ next;
+ }
+
+ # Go to the next line if we are not yet recording LWLocks.
+ next if not $recording_lwlocks;
+
+ # Record the LWLock.
+ (my $waiteventname, my $waitevendocsentence) = split(/\t/, $_);
+ push(@wait_event_lwlocks, $waiteventname . "Lock");
+}
+
+my $i = 0;
while (<$lwlocknames>)
{
chomp;
@@ -50,6 +104,12 @@ while (<$lwlocknames>)
die "lwlocknames.txt not in order" if $lockidx < $lastlockidx;
die "lwlocknames.txt has duplicates" if $lockidx == $lastlockidx;
+ die $lockname . " defined in lwlocknames.txt but missing from wait_event_names.txt"
+ unless $i < scalar(@wait_event_lwlocks);
+ die "lists of predefined LWLocks in lwlocknames.txt and wait_event_names.txt do not match"
+ unless $wait_event_lwlocks[$i] eq $lockname;
+ $i++;
+
while ($lastlockidx < $lockidx - 1)
{
++$lastlockidx;
@@ -63,6 +123,9 @@ while (<$lwlocknames>)
print $h "#define $lockname (&MainLWLockArray[$lockidx].lock)\n";
}
+die $wait_event_lwlocks[$i] . " defined in wait_event_names.txt but missing from lwlocknames.txt"
+ unless scalar(@wait_event_lwlocks) eq $i;
+
printf $c "\n};\n";
print $h "\n";
printf $h "#define NUM_INDIVIDUAL_LWLOCKS %s\n", $lastlockidx + 1;
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 088eb977d4..fed50079fd 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -276,6 +276,10 @@ Extension "Waiting in an extension."
# This class of wait events has its own set of C structure, so these are
# only used for the documentation.
#
+# 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.
+#
+
XactBuffer "Waiting for I/O on a transaction status SLRU buffer."
CommitTsBuffer "Waiting for I/O on a commit timestamp SLRU buffer."
SubtransBuffer "Waiting for I/O on a sub-transaction SLRU buffer."
diff --git a/src/include/storage/meson.build b/src/include/storage/meson.build
index 2a88248464..8df8b404f5 100644
--- a/src/include/storage/meson.build
+++ b/src/include/storage/meson.build
@@ -1,7 +1,7 @@
# Copyright (c) 2022-2024, PostgreSQL Global Development Group
lwlocknames = custom_target('lwlocknames',
- input: files('../../backend/storage/lmgr/lwlocknames.txt'),
+ input: [files('../../backend/storage/lmgr/lwlocknames.txt'), files('../../backend/utils/activity/wait_event_names.txt')],
output: ['lwlocknames.h', 'lwlocknames.c'],
command: [
perl, files('../../backend/storage/lmgr/generate-lwlocknames.pl'),
--
2.25.1
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
Attachments:
v3-0001-verify-lists-of-predefined-LWLocks-in-lwlocknames.patchtext/x-diff; charset=us-asciiDownload
From e1367cb5c9bbad6b963a5f3596a6133df471654c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 5 Jan 2024 00:07:40 -0600
Subject: [PATCH v3 1/1] verify lists of predefined LWLocks in lwlocknames.txt
and wait_event_names.txt match
---
src/backend/Makefile | 2 +-
src/backend/storage/lmgr/Makefile | 4 +-
.../storage/lmgr/generate-lwlocknames.pl | 48 +++++++++++++++++++
.../utils/activity/wait_event_names.txt | 12 +++++
src/include/storage/meson.build | 2 +-
5 files changed, 64 insertions(+), 4 deletions(-)
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 816461aa17..7d2ea7d54a 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -130,7 +130,7 @@ $(top_builddir)/src/port/libpgport_srv.a: | submake-libpgport
parser/gram.h: parser/gram.y
$(MAKE) -C parser gram.h
-storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt
+storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt utils/activity/wait_event_names.txt
$(MAKE) -C storage/lmgr lwlocknames.h lwlocknames.c
utils/activity/wait_event_types.h: utils/activity/generate-wait_event_types.pl utils/activity/wait_event_names.txt
diff --git a/src/backend/storage/lmgr/Makefile b/src/backend/storage/lmgr/Makefile
index c48ba943c4..504480e847 100644
--- a/src/backend/storage/lmgr/Makefile
+++ b/src/backend/storage/lmgr/Makefile
@@ -39,8 +39,8 @@ s_lock_test: s_lock.c $(top_builddir)/src/common/libpgcommon.a $(top_builddir)/s
lwlocknames.c: lwlocknames.h
touch $@
-lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt generate-lwlocknames.pl
- $(PERL) $(srcdir)/generate-lwlocknames.pl $<
+lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt $(top_srcdir)/src/backend/utils/activity/wait_event_names.txt generate-lwlocknames.pl
+ $(PERL) $(srcdir)/generate-lwlocknames.pl $^
check: s_lock_test
./s_lock_test
diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl b/src/backend/storage/lmgr/generate-lwlocknames.pl
index bef5e16e11..237c3b9678 100644
--- a/src/backend/storage/lmgr/generate-lwlocknames.pl
+++ b/src/backend/storage/lmgr/generate-lwlocknames.pl
@@ -15,6 +15,7 @@ my $continue = "\n";
GetOptions('outdir:s' => \$output_path);
open my $lwlocknames, '<', $ARGV[0] or die;
+open my $wait_event_names, '<', $ARGV[1] or die;
# Include PID in suffix in case parallel make runs this multiple times.
my $htmp = "$output_path/lwlocknames.h.tmp$$";
@@ -30,6 +31,42 @@ 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.
+#
+my @wait_event_lwlocks;
+my $record_lwlocks = 0;
+
+while (<$wait_event_names>)
+{
+ chomp;
+
+ # Check for end marker.
+ last if /^# END OF PREDEFINED LWLOCKS/;
+
+ # Skip comments and empty lines.
+ next if /^#/;
+ next if /^\s*$/;
+
+ # Start recording LWLocks when we find the WaitEventLWLock section.
+ if (/^Section: ClassName(.*)/)
+ {
+ my $section_name = $_;
+ $section_name =~ s/^.*- //;
+ $record_lwlocks = 1 if $section_name eq "WaitEventLWLock";
+ next;
+ }
+
+ # Go to the next line if we are not yet recording LWLocks.
+ next if not $record_lwlocks;
+
+ # Record the LWLock.
+ (my $waiteventname, my $waitevendocsentence) = split(/\t/, $_);
+ push(@wait_event_lwlocks, $waiteventname . "Lock");
+}
+
+my $i = 0;
while (<$lwlocknames>)
{
chomp;
@@ -50,6 +87,14 @@ while (<$lwlocknames>)
die "lwlocknames.txt not in order" if $lockidx < $lastlockidx;
die "lwlocknames.txt has duplicates" if $lockidx == $lastlockidx;
+ die $lockname . " defined in lwlocknames.txt but missing from wait_event_names.txt"
+ unless $i < scalar(@wait_event_lwlocks);
+ die "lists of predefined LWLocks do not match (first mismatch at " .
+ $wait_event_lwlocks[$i] . " in wait_event_names.txt and " .
+ $lockname . " in lwlocknames.txt)"
+ unless $wait_event_lwlocks[$i] eq $lockname;
+ $i++;
+
while ($lastlockidx < $lockidx - 1)
{
++$lastlockidx;
@@ -63,6 +108,9 @@ while (<$lwlocknames>)
print $h "#define $lockname (&MainLWLockArray[$lockidx].lock)\n";
}
+die $wait_event_lwlocks[$i] . " defined in wait_event_names.txt but missing from lwlocknames.txt"
+ unless scalar(@wait_event_lwlocks) eq $i;
+
printf $c "\n};\n";
print $h "\n";
printf $h "#define NUM_INDIVIDUAL_LWLOCKS %s\n", $lastlockidx + 1;
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 088eb977d4..4d70e84103 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -276,6 +276,10 @@ Extension "Waiting in an extension."
# This class of wait events has its own set of C structure, so these are
# only used for the documentation.
#
+# 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,14 @@ 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."
+#
+# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
+#
+# 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.
+#
+
XactBuffer "Waiting for I/O on a transaction status SLRU buffer."
CommitTsBuffer "Waiting for I/O on a commit timestamp SLRU buffer."
SubtransBuffer "Waiting for I/O on a sub-transaction SLRU buffer."
diff --git a/src/include/storage/meson.build b/src/include/storage/meson.build
index 2a88248464..8df8b404f5 100644
--- a/src/include/storage/meson.build
+++ b/src/include/storage/meson.build
@@ -1,7 +1,7 @@
# Copyright (c) 2022-2024, PostgreSQL Global Development Group
lwlocknames = custom_target('lwlocknames',
- input: files('../../backend/storage/lmgr/lwlocknames.txt'),
+ input: [files('../../backend/storage/lmgr/lwlocknames.txt'), files('../../backend/utils/activity/wait_event_names.txt')],
output: ['lwlocknames.h', 'lwlocknames.c'],
command: [
perl, files('../../backend/storage/lmgr/generate-lwlocknames.pl'),
--
2.25.1
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
Attachments:
v4-0001-verify-lists-of-predefined-LWLocks-in-lwlocknames.patchtext/x-diff; charset=us-asciiDownload
From 35a744e0114c38e3ffb48e446360b252bb7b17b4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 5 Jan 2024 00:07:40 -0600
Subject: [PATCH v4 1/1] verify lists of predefined LWLocks in lwlocknames.txt
and wait_event_names.txt match
---
src/backend/Makefile | 2 +-
src/backend/storage/lmgr/Makefile | 4 +-
.../storage/lmgr/generate-lwlocknames.pl | 48 +++++++++++++++++++
.../utils/activity/wait_event_names.txt | 12 +++++
src/include/storage/meson.build | 4 +-
5 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 816461aa17..7d2ea7d54a 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -130,7 +130,7 @@ $(top_builddir)/src/port/libpgport_srv.a: | submake-libpgport
parser/gram.h: parser/gram.y
$(MAKE) -C parser gram.h
-storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt
+storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt utils/activity/wait_event_names.txt
$(MAKE) -C storage/lmgr lwlocknames.h lwlocknames.c
utils/activity/wait_event_types.h: utils/activity/generate-wait_event_types.pl utils/activity/wait_event_names.txt
diff --git a/src/backend/storage/lmgr/Makefile b/src/backend/storage/lmgr/Makefile
index c48ba943c4..504480e847 100644
--- a/src/backend/storage/lmgr/Makefile
+++ b/src/backend/storage/lmgr/Makefile
@@ -39,8 +39,8 @@ s_lock_test: s_lock.c $(top_builddir)/src/common/libpgcommon.a $(top_builddir)/s
lwlocknames.c: lwlocknames.h
touch $@
-lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt generate-lwlocknames.pl
- $(PERL) $(srcdir)/generate-lwlocknames.pl $<
+lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt $(top_srcdir)/src/backend/utils/activity/wait_event_names.txt generate-lwlocknames.pl
+ $(PERL) $(srcdir)/generate-lwlocknames.pl $^
check: s_lock_test
./s_lock_test
diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl b/src/backend/storage/lmgr/generate-lwlocknames.pl
index bef5e16e11..237c3b9678 100644
--- a/src/backend/storage/lmgr/generate-lwlocknames.pl
+++ b/src/backend/storage/lmgr/generate-lwlocknames.pl
@@ -15,6 +15,7 @@ my $continue = "\n";
GetOptions('outdir:s' => \$output_path);
open my $lwlocknames, '<', $ARGV[0] or die;
+open my $wait_event_names, '<', $ARGV[1] or die;
# Include PID in suffix in case parallel make runs this multiple times.
my $htmp = "$output_path/lwlocknames.h.tmp$$";
@@ -30,6 +31,42 @@ 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.
+#
+my @wait_event_lwlocks;
+my $record_lwlocks = 0;
+
+while (<$wait_event_names>)
+{
+ chomp;
+
+ # Check for end marker.
+ last if /^# END OF PREDEFINED LWLOCKS/;
+
+ # Skip comments and empty lines.
+ next if /^#/;
+ next if /^\s*$/;
+
+ # Start recording LWLocks when we find the WaitEventLWLock section.
+ if (/^Section: ClassName(.*)/)
+ {
+ my $section_name = $_;
+ $section_name =~ s/^.*- //;
+ $record_lwlocks = 1 if $section_name eq "WaitEventLWLock";
+ next;
+ }
+
+ # Go to the next line if we are not yet recording LWLocks.
+ next if not $record_lwlocks;
+
+ # Record the LWLock.
+ (my $waiteventname, my $waitevendocsentence) = split(/\t/, $_);
+ push(@wait_event_lwlocks, $waiteventname . "Lock");
+}
+
+my $i = 0;
while (<$lwlocknames>)
{
chomp;
@@ -50,6 +87,14 @@ while (<$lwlocknames>)
die "lwlocknames.txt not in order" if $lockidx < $lastlockidx;
die "lwlocknames.txt has duplicates" if $lockidx == $lastlockidx;
+ die $lockname . " defined in lwlocknames.txt but missing from wait_event_names.txt"
+ unless $i < scalar(@wait_event_lwlocks);
+ die "lists of predefined LWLocks do not match (first mismatch at " .
+ $wait_event_lwlocks[$i] . " in wait_event_names.txt and " .
+ $lockname . " in lwlocknames.txt)"
+ unless $wait_event_lwlocks[$i] eq $lockname;
+ $i++;
+
while ($lastlockidx < $lockidx - 1)
{
++$lastlockidx;
@@ -63,6 +108,9 @@ while (<$lwlocknames>)
print $h "#define $lockname (&MainLWLockArray[$lockidx].lock)\n";
}
+die $wait_event_lwlocks[$i] . " defined in wait_event_names.txt but missing from lwlocknames.txt"
+ unless scalar(@wait_event_lwlocks) eq $i;
+
printf $c "\n};\n";
print $h "\n";
printf $h "#define NUM_INDIVIDUAL_LWLOCKS %s\n", $lastlockidx + 1;
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 088eb977d4..4d70e84103 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -276,6 +276,10 @@ Extension "Waiting in an extension."
# This class of wait events has its own set of C structure, so these are
# only used for the documentation.
#
+# 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,14 @@ 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."
+#
+# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
+#
+# 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.
+#
+
XactBuffer "Waiting for I/O on a transaction status SLRU buffer."
CommitTsBuffer "Waiting for I/O on a commit timestamp SLRU buffer."
SubtransBuffer "Waiting for I/O on a sub-transaction SLRU buffer."
diff --git a/src/include/storage/meson.build b/src/include/storage/meson.build
index 2a88248464..7f1ce38566 100644
--- a/src/include/storage/meson.build
+++ b/src/include/storage/meson.build
@@ -1,7 +1,9 @@
# Copyright (c) 2022-2024, PostgreSQL Global Development Group
lwlocknames = custom_target('lwlocknames',
- input: files('../../backend/storage/lmgr/lwlocknames.txt'),
+ input: [files(
+ '../../backend/storage/lmgr/lwlocknames.txt',
+ '../../backend/utils/activity/wait_event_names.txt')],
output: ['lwlocknames.h', 'lwlocknames.c'],
command: [
perl, files('../../backend/storage/lmgr/generate-lwlocknames.pl'),
--
2.25.1
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
Attachments:
v5-0001-verify-lists-of-predefined-LWLocks-in-lwlocknames.patchtext/x-diff; charset=us-asciiDownload
From 53c492ac8b345eca1c5c245882073273ddde53c1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 5 Jan 2024 00:07:40 -0600
Subject: [PATCH v5 1/1] verify lists of predefined LWLocks in lwlocknames.txt
and wait_event_names.txt match
---
src/backend/Makefile | 2 +-
src/backend/storage/lmgr/Makefile | 4 +-
.../storage/lmgr/generate-lwlocknames.pl | 48 +++++++++++++++++++
.../utils/activity/wait_event_names.txt | 12 +++++
src/include/storage/meson.build | 4 +-
5 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 816461aa17..7d2ea7d54a 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -130,7 +130,7 @@ $(top_builddir)/src/port/libpgport_srv.a: | submake-libpgport
parser/gram.h: parser/gram.y
$(MAKE) -C parser gram.h
-storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt
+storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt utils/activity/wait_event_names.txt
$(MAKE) -C storage/lmgr lwlocknames.h lwlocknames.c
utils/activity/wait_event_types.h: utils/activity/generate-wait_event_types.pl utils/activity/wait_event_names.txt
diff --git a/src/backend/storage/lmgr/Makefile b/src/backend/storage/lmgr/Makefile
index c48ba943c4..504480e847 100644
--- a/src/backend/storage/lmgr/Makefile
+++ b/src/backend/storage/lmgr/Makefile
@@ -39,8 +39,8 @@ s_lock_test: s_lock.c $(top_builddir)/src/common/libpgcommon.a $(top_builddir)/s
lwlocknames.c: lwlocknames.h
touch $@
-lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt generate-lwlocknames.pl
- $(PERL) $(srcdir)/generate-lwlocknames.pl $<
+lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt $(top_srcdir)/src/backend/utils/activity/wait_event_names.txt generate-lwlocknames.pl
+ $(PERL) $(srcdir)/generate-lwlocknames.pl $^
check: s_lock_test
./s_lock_test
diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl b/src/backend/storage/lmgr/generate-lwlocknames.pl
index bef5e16e11..237c3b9678 100644
--- a/src/backend/storage/lmgr/generate-lwlocknames.pl
+++ b/src/backend/storage/lmgr/generate-lwlocknames.pl
@@ -15,6 +15,7 @@ my $continue = "\n";
GetOptions('outdir:s' => \$output_path);
open my $lwlocknames, '<', $ARGV[0] or die;
+open my $wait_event_names, '<', $ARGV[1] or die;
# Include PID in suffix in case parallel make runs this multiple times.
my $htmp = "$output_path/lwlocknames.h.tmp$$";
@@ -30,6 +31,42 @@ 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.
+#
+my @wait_event_lwlocks;
+my $record_lwlocks = 0;
+
+while (<$wait_event_names>)
+{
+ chomp;
+
+ # Check for end marker.
+ last if /^# END OF PREDEFINED LWLOCKS/;
+
+ # Skip comments and empty lines.
+ next if /^#/;
+ next if /^\s*$/;
+
+ # Start recording LWLocks when we find the WaitEventLWLock section.
+ if (/^Section: ClassName(.*)/)
+ {
+ my $section_name = $_;
+ $section_name =~ s/^.*- //;
+ $record_lwlocks = 1 if $section_name eq "WaitEventLWLock";
+ next;
+ }
+
+ # Go to the next line if we are not yet recording LWLocks.
+ next if not $record_lwlocks;
+
+ # Record the LWLock.
+ (my $waiteventname, my $waitevendocsentence) = split(/\t/, $_);
+ push(@wait_event_lwlocks, $waiteventname . "Lock");
+}
+
+my $i = 0;
while (<$lwlocknames>)
{
chomp;
@@ -50,6 +87,14 @@ while (<$lwlocknames>)
die "lwlocknames.txt not in order" if $lockidx < $lastlockidx;
die "lwlocknames.txt has duplicates" if $lockidx == $lastlockidx;
+ die $lockname . " defined in lwlocknames.txt but missing from wait_event_names.txt"
+ unless $i < scalar(@wait_event_lwlocks);
+ die "lists of predefined LWLocks do not match (first mismatch at " .
+ $wait_event_lwlocks[$i] . " in wait_event_names.txt and " .
+ $lockname . " in lwlocknames.txt)"
+ unless $wait_event_lwlocks[$i] eq $lockname;
+ $i++;
+
while ($lastlockidx < $lockidx - 1)
{
++$lastlockidx;
@@ -63,6 +108,9 @@ while (<$lwlocknames>)
print $h "#define $lockname (&MainLWLockArray[$lockidx].lock)\n";
}
+die $wait_event_lwlocks[$i] . " defined in wait_event_names.txt but missing from lwlocknames.txt"
+ unless scalar(@wait_event_lwlocks) eq $i;
+
printf $c "\n};\n";
print $h "\n";
printf $h "#define NUM_INDIVIDUAL_LWLOCKS %s\n", $lastlockidx + 1;
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 088eb977d4..4d70e84103 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -276,6 +276,10 @@ Extension "Waiting in an extension."
# This class of wait events has its own set of C structure, so these are
# only used for the documentation.
#
+# 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,14 @@ 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."
+#
+# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
+#
+# 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.
+#
+
XactBuffer "Waiting for I/O on a transaction status SLRU buffer."
CommitTsBuffer "Waiting for I/O on a commit timestamp SLRU buffer."
SubtransBuffer "Waiting for I/O on a sub-transaction SLRU buffer."
diff --git a/src/include/storage/meson.build b/src/include/storage/meson.build
index 2a88248464..666fb22408 100644
--- a/src/include/storage/meson.build
+++ b/src/include/storage/meson.build
@@ -1,7 +1,9 @@
# Copyright (c) 2022-2024, PostgreSQL Global Development Group
lwlocknames = custom_target('lwlocknames',
- input: files('../../backend/storage/lmgr/lwlocknames.txt'),
+ input: files(
+ '../../backend/storage/lmgr/lwlocknames.txt',
+ '../../backend/utils/activity/wait_event_names.txt'),
output: ['lwlocknames.h', 'lwlocknames.c'],
command: [
perl, files('../../backend/storage/lmgr/generate-lwlocknames.pl'),
--
2.25.1
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
Attachments:
v6-0001-Cross-check-lists-of-predefined-LWLocks.patchtext/x-diff; charset=us-asciiDownload
From 4e222c1a1fc1a2476746cb7b68c1d2a203816699 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 8 Jan 2024 15:44:44 -0600
Subject: [PATCH v6 1/1] Cross-check lists of predefined LWLocks.
Both lwlocknames.txt and wait_event_names.txt contain a list of all
the predefined LWLocks, i.e., those with predefined positions
within MainLWLockArray. It is easy to miss one or the other,
especially since the list in wait_event_names.txt omits the "Lock"
suffix for all the LWLock wait events. This commit adds a cross-
check of these lists to the script that generates lwlocknames.h.
If the lists do not match exactly, building will fail.
Suggested-by: Robert Haas
Reviewed-by: Robert Haas, Michael Paquier, Bertrand Drouvot
Discussion: https://postgr.es/m/20240102173120.GA1061678%40nathanxps13
---
src/backend/Makefile | 2 +-
src/backend/storage/lmgr/Makefile | 4 +-
.../storage/lmgr/generate-lwlocknames.pl | 49 +++++++++++++++++++
.../utils/activity/wait_event_names.txt | 12 +++++
src/include/storage/meson.build | 4 +-
5 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 816461aa17..7d2ea7d54a 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -130,7 +130,7 @@ $(top_builddir)/src/port/libpgport_srv.a: | submake-libpgport
parser/gram.h: parser/gram.y
$(MAKE) -C parser gram.h
-storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt
+storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt utils/activity/wait_event_names.txt
$(MAKE) -C storage/lmgr lwlocknames.h lwlocknames.c
utils/activity/wait_event_types.h: utils/activity/generate-wait_event_types.pl utils/activity/wait_event_names.txt
diff --git a/src/backend/storage/lmgr/Makefile b/src/backend/storage/lmgr/Makefile
index c48ba943c4..504480e847 100644
--- a/src/backend/storage/lmgr/Makefile
+++ b/src/backend/storage/lmgr/Makefile
@@ -39,8 +39,8 @@ s_lock_test: s_lock.c $(top_builddir)/src/common/libpgcommon.a $(top_builddir)/s
lwlocknames.c: lwlocknames.h
touch $@
-lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt generate-lwlocknames.pl
- $(PERL) $(srcdir)/generate-lwlocknames.pl $<
+lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt $(top_srcdir)/src/backend/utils/activity/wait_event_names.txt generate-lwlocknames.pl
+ $(PERL) $(srcdir)/generate-lwlocknames.pl $^
check: s_lock_test
./s_lock_test
diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl b/src/backend/storage/lmgr/generate-lwlocknames.pl
index bef5e16e11..06401dbf01 100644
--- a/src/backend/storage/lmgr/generate-lwlocknames.pl
+++ b/src/backend/storage/lmgr/generate-lwlocknames.pl
@@ -15,6 +15,7 @@ my $continue = "\n";
GetOptions('outdir:s' => \$output_path);
open my $lwlocknames, '<', $ARGV[0] or die;
+open my $wait_event_names, '<', $ARGV[1] or die;
# Include PID in suffix in case parallel make runs this multiple times.
my $htmp = "$output_path/lwlocknames.h.tmp$$";
@@ -30,6 +31,40 @@ 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.
+#
+my @wait_event_lwlocks;
+my $record_lwlocks = 0;
+
+while (<$wait_event_names>)
+{
+ chomp;
+
+ # Check for end marker.
+ last if /^# END OF PREDEFINED LWLOCKS/;
+
+ # Skip comments and empty lines.
+ next if /^#/;
+ next if /^\s*$/;
+
+ # Start recording LWLocks when we find the WaitEventLWLock section.
+ if (/^Section: ClassName - WaitEventLWLock$/)
+ {
+ $record_lwlocks = 1;
+ next;
+ }
+
+ # Go to the next line if we are not yet recording LWLocks.
+ next if not $record_lwlocks;
+
+ # Record the LWLock.
+ (my $waiteventname, my $waitevendocsentence) = split(/\t/, $_);
+ push(@wait_event_lwlocks, $waiteventname . "Lock");
+}
+
+my $i = 0;
while (<$lwlocknames>)
{
chomp;
@@ -50,6 +85,15 @@ while (<$lwlocknames>)
die "lwlocknames.txt not in order" if $lockidx < $lastlockidx;
die "lwlocknames.txt has duplicates" if $lockidx == $lastlockidx;
+ die "$lockname defined in lwlocknames.txt but missing from "
+ . "wait_event_names.txt"
+ if $i >= scalar @wait_event_lwlocks;
+ die "lists of predefined LWLocks do not match (first mismatch at "
+ . "$wait_event_lwlocks[$i] in wait_event_names.txt and $lockname in "
+ . "lwlocknames.txt)"
+ if $wait_event_lwlocks[$i] ne $lockname;
+ $i++;
+
while ($lastlockidx < $lockidx - 1)
{
++$lastlockidx;
@@ -63,6 +107,11 @@ while (<$lwlocknames>)
print $h "#define $lockname (&MainLWLockArray[$lockidx].lock)\n";
}
+die
+ "$wait_event_lwlocks[$i] defined in wait_event_names.txt but missing from "
+ . "lwlocknames.txt"
+ if scalar @wait_event_lwlocks ne $i;
+
printf $c "\n};\n";
print $h "\n";
printf $h "#define NUM_INDIVIDUAL_LWLOCKS %s\n", $lastlockidx + 1;
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 088eb977d4..ad6bf0de84 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -276,6 +276,10 @@ Extension "Waiting in an extension."
# This class of wait events has its own set of C structure, so these are
# only used for the documentation.
#
+# NB: Predefined locks (i.e., 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,14 @@ 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."
+#
+# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
+#
+# Predefined LWLocks (i.e., 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.
+#
+
XactBuffer "Waiting for I/O on a transaction status SLRU buffer."
CommitTsBuffer "Waiting for I/O on a commit timestamp SLRU buffer."
SubtransBuffer "Waiting for I/O on a sub-transaction SLRU buffer."
diff --git a/src/include/storage/meson.build b/src/include/storage/meson.build
index 2a88248464..666fb22408 100644
--- a/src/include/storage/meson.build
+++ b/src/include/storage/meson.build
@@ -1,7 +1,9 @@
# Copyright (c) 2022-2024, PostgreSQL Global Development Group
lwlocknames = custom_target('lwlocknames',
- input: files('../../backend/storage/lmgr/lwlocknames.txt'),
+ input: files(
+ '../../backend/storage/lmgr/lwlocknames.txt',
+ '../../backend/utils/activity/wait_event_names.txt'),
output: ['lwlocknames.h', 'lwlocknames.c'],
command: [
perl, files('../../backend/storage/lmgr/generate-lwlocknames.pl'),
--
2.25.1
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