Date: Fri, 5 Jan 2024 11:46:20 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	Michael Paquier <michael@paquier.xyz>
Subject: Re: verify predefined LWLocks have entries in wait_event_names.txt
Message-ID: <20240105174620.GA2285561@nathanxps13>
References: <20240102173120.GA1061678@nathanxps13>
 <CA+TgmoYRhhL12iOcNKv6f__iZMk1_BPgbASFykPazhr_bwk0=w@mail.gmail.com>
 <20240102214544.GA1063425@nathanxps13>
 <CA+TgmoZGDO_hx8QvFkgj29JjyWm1+YJRELySwuLqiTJymAGVSw@mail.gmail.com>
 <ZZUT8SqhoUyy/hhM@ip-10-97-1-34.eu-west-3.compute.internal>
 <20240105061144.GA2166447@nathanxps13>
 <ZZeyO4eMjR2eiIiP@ip-10-97-1-34.eu-west-3.compute.internal>
 <20240105164203.GC2168314@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="mYCpIKhGyMATD0i+"
Content-Disposition: inline
In-Reply-To: <20240105164203.GC2168314@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/20240105174620.GA2285561%40nathanxps13>
Precedence: bulk


--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--mYCpIKhGyMATD0i+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-verify-lists-of-predefined-LWLocks-in-lwlocknames.patch"

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


--mYCpIKhGyMATD0i+--