pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedAdd AioUringCompletion in wait_event_names.txt and a safeguard in generate-wait_event_types.pl
4+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2025-05-29 c3623703f3 Add AioUringCompletion in wait_event_names.txt
* Add AioUringCompletion in wait_event_names.txt and a safeguard in generate-wait_event_types.pl
@ 2025-05-26 07:49 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Bertrand Drouvot @ 2025-05-26 07:49 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
Hi hackers,
c325a7633fcb forgot to add AioUringCompletion in wait_event_names.txt: please
find attached v1-0001 fixing it (please double check the wait event description
as I'm not that familiar with io_uring).
042a66291 also missed it for LWTRANCHE_MEMORY_CONTEXT_REPORTING_PROC and
LWTRANCHE_MEMORY_CONTEXT_REPORTING_STATE. 042a66291 has been reverted in
fb844b9f065 though so there is no need to fix those.
That makes me think that it is easy to miss adding a new LWLock in
wait_event_names.txt and generate-wait_event_types.pl does not detect that.
So, adding validate_lwlock_count() in generate-wait_event_types.pl
(in v1-0002) to ensure that the number of LWLocks description generated in the
documentation match the ones defined in lwlock.h and lwlocklist.h.
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments: download all (2 files, .tar.gz)
v1-0001-Add-AioUringCompletion-in-wait_event_names.txt.patch (text/x-diff, 1.1K) download
From 9a86692ab335ccddf523d9e7685b6e5f700578bf Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Mon, 26 May 2025 05:20:37 +0000
Subject: [PATCH v1 1/2] Add AioUringCompletion in wait_event_names.txt
c325a7633fcb forgot to add AioUringCompletion in wait_event_names.txt.
---
src/backend/utils/activity/wait_event_names.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 5d9e04d6823..4da68312b5f 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -401,6 +401,7 @@ SerialSLRU "Waiting to access the serializable transaction conflict SLRU cache."
SubtransSLRU "Waiting to access the sub-transaction SLRU cache."
XactSLRU "Waiting to access the transaction status SLRU cache."
ParallelVacuumDSA "Waiting for parallel vacuum dynamic shared memory allocation."
+AioUringCompletion "Waiting for another process to complete IO via io_uring."
# No "ABI_compatibility" region here as WaitEventLWLock has its own C code.
--
2.34.1
v1-0002-Add-validate_lwlock_count-in-generate-wait_event_.patch (text/x-diff, 7.4K) download
From a00d327f5ee47b80e17aa61968440c62039c682d Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Mon, 26 May 2025 06:52:10 +0000
Subject: [PATCH v1 2/2] Add validate_lwlock_count() in
generate-wait_event_types.pl
It's easy to miss to add new LWLock(s) in wait_event_names.txt, so adding a
function in generate-wait_event_types.pl to validate that no LWLock is missing
in wait_event_names.txt (comparing with the ones defined in lwlock.h and
lwlocklist.h).
---
doc/src/sgml/Makefile | 4 +-
doc/src/sgml/meson.build | 2 +-
.../activity/generate-wait_event_types.pl | 102 ++++++++++++++++++
src/include/storage/lwlock.h | 3 +
src/include/storage/lwlocklist.h | 4 +-
5 files changed, 111 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/Makefile b/doc/src/sgml/Makefile
index 11aac913812..aabb3c116e7 100644
--- a/doc/src/sgml/Makefile
+++ b/doc/src/sgml/Makefile
@@ -109,8 +109,8 @@ errcodes-table.sgml: $(top_srcdir)/src/backend/utils/errcodes.txt generate-errco
keywords-table.sgml: $(top_srcdir)/src/include/parser/kwlist.h $(wildcard $(srcdir)/keywords/sql*.txt) generate-keywords-table.pl
$(PERL) $(srcdir)/generate-keywords-table.pl $(srcdir) > $@
-wait_event_types.sgml: $(top_srcdir)/src/backend/utils/activity/wait_event_names.txt $(top_srcdir)/src/backend/utils/activity/generate-wait_event_types.pl
- $(PERL) $(top_srcdir)/src/backend/utils/activity/generate-wait_event_types.pl --docs $<
+wait_event_types.sgml: $(top_srcdir)/src/backend/utils/activity/wait_event_names.txt $(top_srcdir)/src/include/storage/lwlock.h $(top_srcdir)/src/include/storage/lwlocklist.h $(top_srcdir)/src/backend/utils/activity/generate-wait_event_types.pl
+ $(PERL) $(top_srcdir)/src/backend/utils/activity/generate-wait_event_types.pl --docs $(wordlist 1,3,$^)
targets-meson.sgml: targets-meson.txt $(srcdir)/generate-targets-meson.pl
$(PERL) $(srcdir)/generate-targets-meson.pl $^ > $@
diff --git a/doc/src/sgml/meson.build b/doc/src/sgml/meson.build
index 6ae192eac68..386695b7029 100644
--- a/doc/src/sgml/meson.build
+++ b/doc/src/sgml/meson.build
@@ -48,7 +48,7 @@ doc_generated += custom_target('errcodes-table.sgml',
doc_generated += custom_target('wait_event_types.sgml',
input: files(
- '../../../src/backend/utils/activity/wait_event_names.txt'),
+ '../../../src/backend/utils/activity/wait_event_names.txt','../../../src/include/storage/lwlock.h','../../../src/include/storage/lwlocklist.h'),
output: 'wait_event_types.sgml',
command: [perl,
files('../../../src/backend/utils/activity/generate-wait_event_types.pl'),
diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl
index 424ad9f115d..b43da8c4dbe 100644
--- a/src/backend/utils/activity/generate-wait_event_types.pl
+++ b/src/backend/utils/activity/generate-wait_event_types.pl
@@ -36,8 +36,107 @@ die "Needs to specify --docs or --code"
die "Not possible to specify --docs and --code simultaneously"
if ($gen_docs && $gen_code);
+# Check arguments based on mode
+if ($gen_docs)
+{
+ die "Usage: $0 --docs wait_event_names.txt lwlock.h lwlocklist.h\n"
+ if (@ARGV != 3);
+}
+else
+{
+ die "Usage: $0 --code wait_event_names.txt\n"
+ if (@ARGV != 1);
+}
+
open my $wait_event_names, '<', $ARGV[0] or die;
+# LWLock count validation function to ensure that all the LWLocks are documented
+sub validate_lwlock_count
+{
+ my ($lwlock_h_file, $lwlocklist_h_file) = @_;
+
+ unless (-f $lwlock_h_file)
+ {
+ die "ERROR: Cannot find lwlock.h file: $lwlock_h_file\n";
+ }
+
+ unless (-f $lwlocklist_h_file)
+ {
+ die "ERROR: Cannot find lwlocklist.h file: $lwlocklist_h_file\n";
+ }
+
+ # Parse BuiltinTrancheIds enum from lwlock.h to count entries
+ open(my $h_fh, '<', $lwlock_h_file);
+
+ my $builtin_tranche_ids_count = 0;
+ my $in_builtin_tranche_ids = 0;
+
+ while (my $line = <$h_fh>)
+ {
+ chomp $line;
+
+ # Look for the start of BuiltinTrancheIds enum
+ if ($line =~ /typedef\s+enum\s+BuiltinTrancheIds/)
+ {
+ $in_builtin_tranche_ids = 1;
+ next;
+ }
+
+ if ($in_builtin_tranche_ids)
+ {
+ # Count LWTRANCHE_ entries (excluding LWTRANCHE_FIRST_USER_DEFINED)
+ if ($line =~ /^\s*LWTRANCHE_\w+/ && $line !~ /FIRST_USER_DEFINED/)
+ {
+ $builtin_tranche_ids_count++;
+ }
+
+ # End of enum
+ if ($line =~ /^\s*\}\s*BuiltinTrancheIds;/)
+ {
+ last;
+ }
+ }
+ }
+ close($h_fh);
+
+ # Parse lwlocklist.h to count PG_LWLOCK entries
+ open(my $list_fh, '<', $lwlocklist_h_file);
+
+ my $lwlocklist_count = 0;
+ while (my $line = <$list_fh>)
+ {
+ chomp $line;
+ # Count PG_LWLOCK entries
+ if ($line =~ /^\s*PG_LWLOCK\s*\(/)
+ {
+ $lwlocklist_count++;
+ }
+ }
+ close($list_fh);
+
+ # Total expected LWLock count is lwlocklist + builtin tranches
+ my $total_expected_count = $lwlocklist_count + $builtin_tranche_ids_count;
+
+ # Get LWLock events count from our parsed wait events
+ my $lwlock_wait_events_count = 0;
+ if (exists $hashwe{'WaitEventLWLock'})
+ {
+ $lwlock_wait_events_count = scalar(@{ $hashwe{'WaitEventLWLock'} });
+ }
+
+ # Count validation
+ if ($total_expected_count != $lwlock_wait_events_count)
+ {
+ die "ERROR: LWLock count mismatch!\n"
+ . " PG_LWLOCK entries in lwlocklist.h: $lwlocklist_count entries\n"
+ . " BuiltinTrancheIds in lwlock.h: $builtin_tranche_ids_count entries\n"
+ . " Total expected: $total_expected_count entries\n"
+ . " WaitEventLWLock in wait_event_names.txt: $lwlock_wait_events_count entries\n"
+ . "\nThe counts do not match. Please ensure all LWLock entries\n"
+ . "have corresponding entries in the LWLock section of wait_event_names.txt.\n";
+ }
+}
+
my @abi_compatibility_lines;
my @lines;
my $abi_compatibility = 0;
@@ -284,6 +383,9 @@ if ($gen_code)
# Generate the .sgml file.
elsif ($gen_docs)
{
+ # Validate LWLock count when generating documentation
+ validate_lwlock_count($ARGV[1], $ARGV[2]);
+
# Include PID in suffix in case parallel make runs this multiple times.
my $stmp = "$output_path/wait_event_names.s.tmp$$";
open my $s, '>', $stmp or die "Could not open $stmp: $!";
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 08a72569ae5..5343350b795 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -7,6 +7,9 @@
* Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
+ * generate-wait_event_types.pl processes this file to ensure that no
+ * BuiltinTrancheIds are missed in wait_event_names.txt.
+ *
* src/include/storage/lwlock.h
*
*-------------------------------------------------------------------------
diff --git a/src/include/storage/lwlocklist.h b/src/include/storage/lwlocklist.h
index a9681738146..6fcb126d034 100644
--- a/src/include/storage/lwlocklist.h
+++ b/src/include/storage/lwlocklist.h
@@ -7,7 +7,9 @@
* the PG_LWLOCK macro, which is not defined in this file; it can be
* defined by the caller for special purposes.
*
- * Also, generate-lwlocknames.pl processes this file to create lwlocknames.h.
+ * Also, generate-lwlocknames.pl processes this file to create lwlocknames.h
+ * and generate-wait_event_types.pl processes this file to ensure that
+ * no predefined LWLock is missed in wait_event_names.txt.
*
* Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
--
2.34.1
* Re: Add AioUringCompletion in wait_event_names.txt and a safeguard in generate-wait_event_types.pl
@ 2025-05-26 23:30 Michael Paquier <michael@paquier.xyz>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 2 replies; 4+ messages in thread
From: Michael Paquier @ 2025-05-26 23:30 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Mon, May 26, 2025 at 07:49:34AM +0000, Bertrand Drouvot wrote:
> c325a7633fcb forgot to add AioUringCompletion in wait_event_names.txt: please
> find attached v1-0001 fixing it (please double check the wait event description
> as I'm not that familiar with io_uring).
+AioUringCompletion "Waiting for another process to complete IO via io_uring."
"completion_lock" is described with similar words around the top of
method_io_uring.c. In more exact words, we are waiting for a backend
to process IO completions with io_method=io_uring. So perhaps:
"Waiting for another process to process IO completions with io_uring"
Adding Andres in CC for input, as I suspect that he'll think about a
better wording for the docs than what I could come up with.
> That makes me think that it is easy to miss adding a new LWLock in
> wait_event_names.txt and generate-wait_event_types.pl does not detect that.
Agreed, adding a check is a good idea seeing that the same mistake has
been repeated two times in the last couple of months by two different
committers. This check is only in the docs, but the CI would detect
that. This is not something strictly required for v18. Let's tackle
that in v19~.
+ if (exists $hashwe{'WaitEventLWLock'})
So, the only reason why this is included in
generate-wait_event_types.pl is that we need the data from %hashwe,
reused in the new function you have added to perform the validation.
The implementation of your validation check is not consistent with the
existing generate-wait_event_types.pl, adding knowledge about lwlock.h
lwlocklist.h which are passed as extra arguments for the function
validate_lwlock_count(). Perhaps it would be better if split into its
own script, with the code in charge of building %hashwe (aka parsing
the .txt file given in input argument) moved to a .pm "Parsing"
module shared by both scripts, the one generating the data and the one
cross-checking the lwlock numbers?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Add AioUringCompletion in wait_event_names.txt and a safeguard in generate-wait_event_types.pl
@ 2025-05-29 04:31 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 4+ messages in thread
From: Michael Paquier @ 2025-05-29 04:31 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Tue, May 27, 2025 at 08:30:56AM +0900, Michael Paquier wrote:
> +AioUringCompletion "Waiting for another process to complete IO via io_uring."
>
> "completion_lock" is described with similar words around the top of
> method_io_uring.c. In more exact words, we are waiting for a backend
> to process IO completions with io_method=io_uring. So perhaps:
> "Waiting for another process to process IO completions with io_uring"
Hearing nothing, I've applied this one reusing the exact wording
suggested by Bertrand which is the most consistent choice with the
other wait events related to the same area of the code.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Add AioUringCompletion in wait_event_names.txt and a safeguard in generate-wait_event_types.pl
@ 2025-06-02 04:59 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 4+ messages in thread
From: Bertrand Drouvot @ 2025-06-02 04:59 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers@lists.postgresql.org, Andres Freund <andres@anarazel.de>
Hi,
On Tue, May 27, 2025 at 08:30:56AM +0900, Michael Paquier wrote:
> On Mon, May 26, 2025 at 07:49:34AM +0000, Bertrand Drouvot wrote:
> > That makes me think that it is easy to miss adding a new LWLock in
> > wait_event_names.txt and generate-wait_event_types.pl does not detect that.
>
> Agreed, adding a check is a good idea
Thanks for sharing your thoughts! Yeah and avoid code/doc discrepancies was
also one of the reason for generate-wait_event_types.pl creation, so I think
that we must ensure that it does so.
> This check is only in the docs, but the CI would detect
> that.
Right.
> This is not something strictly required for v18. Let's tackle
> that in v19~.
Yes.
> + if (exists $hashwe{'WaitEventLWLock'})
>
> So, the only reason why this is included in
> generate-wait_event_types.pl is that we need the data from %hashwe,
> reused in the new function you have added to perform the validation.
>
> The implementation of your validation check is not consistent with the
> existing generate-wait_event_types.pl, adding knowledge about lwlock.h
> lwlocklist.h which are passed as extra arguments for the function
> validate_lwlock_count().
I'm not sure to get what you mean with "not consistent". I think that we need
lwlock.h and lwlocklist.h so that generate-wait_event_types.pl can do its
work efficiently i.e generate the code, doc and avoid code/doc discrepancies.
> Perhaps it would be better if split into its
> own script, with the code in charge of building %hashwe (aka parsing
> the .txt file given in input argument) moved to a .pm "Parsing"
> module shared by both scripts, the one generating the data and the one
> cross-checking the lwlock numbers?
That could probably work, but do we really need building a new .pl file? I think
that it would make more sense if the "new" .pl in charge of cross-checking the
lwlock numbers would do more checks (unrelated to generate-wait_event_types.pl).
Then, we could have something like the .pm, check_generated_code.pl (new pl) and
generate-wait_event_types.pl with check_generated_code.pl doing things that are
related or not to generate-wait_event_types.pl.
I feel that what is proposed here is really linked to generate-wait_event_types.pl
"only" (it's part of its duties) and that's probably why I'm having some difficulty
to see the pros of the split.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2025-06-02 04:59 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-05-26 07:49 Add AioUringCompletion in wait_event_names.txt and a safeguard in generate-wait_event_types.pl Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2025-05-26 23:30 ` Michael Paquier <michael@paquier.xyz>
2025-05-29 04:31 ` Michael Paquier <michael@paquier.xyz>
2025-06-02 04:59 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox