Hi,
On Thu, Apr 04, 2024 at 03:50:21PM +0900, Michael Paquier wrote:
> On Tue, Mar 19, 2024 at 07:34:09AM +0000, Bertrand Drouvot wrote:
> > I'm not sure as v2 used the "version >= 17" wording which I think would not
> > need
> > manual refresh each time a new stable branch is forked.
> >
> > But to avoid any doubt, I'm following your recommendation in v3 attached
> > (then
> > only mentioning the "master branch" and "any other branch").
>
> I don't see why we could not be more generic, TBH. Note that the
> Backpatch region should be empty not only the master branch but also
> on stable and unreleased branches (aka REL_XX_STABLE branches from
> their fork from master to their .0 release). I have reworded the
> whole, mentioning ABI compatibility, as well.
Yeah, agree. I do prefer your wording.
> The position of the Backpatch regions were a bit incorrect (extra one
> in LWLock, and the one in Lock was not needed).
oops, thanks for the fixes!
> We could be stricter with the order of the elements in
> pgstat_wait_event.c and wait_event_funcs_data.c, but there's no
> consequence feature-wise and I cannot get excited about the extra
> complexity this creates in generate-wait_event_types.pl between the
> enum generation and the rest.
Yeah, and I think generate-wait_event_types.pl is already complex enough.
So better to add only the strict necessary in it IMHO.
> Is "Backpatch" the best choice we have, though? It speaks by itself
> but I was thinking about something different, like "Stable". Other
> ideas or objections are welcome. My naming sense is usually not that
> good, so there's that.
I think "Stable" is more confusing because the section should also be empty
until
the .0 is released.
That said, what about "ABI_compatibility"? (that would also match the comment
added in wait_event_names.txt). Attached v4 making use of the ABI_compatibility
proposal.
> 0001 is the patch with my tweaks.
Thanks!
+# No "Backpatch" region here as code is generated automatically.
What about "....region here as has its own C code" (that would be more
consistent
with the comment in the "header" for the file). Done that way in v4.
It looks like WAL_SENDER_WRITE_ZZZ was also added in it (I guess for testing
purpose, so I removed it in v4).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From 4a69b8adb9bef2a5c7f7bec061d0bdda95ad9e24 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 4 Apr 2024 15:34:37 +0900
Subject: [PATCH v4] Add "ABI_compatibility" regions in wait_event_names.txt
When backpatching, adding an event will renumber others, which can make an
extension report the wrong event until recompiled. This is due to the fact that
generate-wait_event_types.pl sorts events to position them. The "ABI_compatibility"
region added here ensures no ordering is done for the wait events found after
this delimiter.
---
.../activity/generate-wait_event_types.pl | 27 ++++++++++++++++++-
.../utils/activity/wait_event_names.txt | 17 ++++++++++++
2 files changed, 43 insertions(+), 1 deletion(-)
100.0% src/backend/utils/activity/
diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl
index f1adf0e8e7..9768406db5 100644
--- a/src/backend/utils/activity/generate-wait_event_types.pl
+++ b/src/backend/utils/activity/generate-wait_event_types.pl
@@ -38,7 +38,9 @@ die "Not possible to specify --docs and --code simultaneously"
open my $wait_event_names, '<', $ARGV[0] or die;
+my @abi_compatibility_lines;
my @lines;
+my $abi_compatibility = 0;
my $section_name;
my $note;
my $note_name;
@@ -59,10 +61,27 @@ while (<$wait_event_names>)
{
$section_name = $_;
$section_name =~ s/^.*- //;
+ $abi_compatibility = 0;
next;
}
- push(@lines, $section_name . "\t" . $_);
+ # ABI_compatibility region, preserving ABI compatibility of the code
+ # generated. Any wait events listed in this part of the file
+ # will not be sorted during the code generation.
+ if (/^ABI_compatibility:$/)
+ {
+ $abi_compatibility = 1;
+ next;
+ }
+
+ if ($gen_code && $abi_compatibility)
+ {
+ push(@abi_compatibility_lines, $section_name . "\t" . $_);
+ }
+ else
+ {
+ push(@lines, $section_name . "\t" . $_);
+ }
}
# Sort the lines based on the second column.
@@ -70,6 +89,12 @@ while (<$wait_event_names>)
my @lines_sorted =
sort { uc((split(/\t/, $a))[1]) cmp uc((split(/\t/, $b))[1]) } @lines;
+# If we are generating code, concat @lines_sorted and then @abi_compatibility_lines.
+if ($gen_code)
+{
+ push(@lines_sorted, @abi_compatibility_lines);
+}
+
# Read the sorted lines and populate the hash table
foreach my $line (@lines_sorted)
{
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 0d288d6b3d..fbb5dae5ea 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -26,6 +26,12 @@
# When adding a new wait event, make sure it is placed in the appropriate
# ClassName section.
#
+# Wait events added in stable branches should be appended to the lists in
+# the "ABI_compatibility:" region of their related ClassName section to preserve
+# ABI compatibility of the C code generated from this file's data, respecting
+# the order of any wait event already listed there. The "ABI_compatibility:"
+# regions should remain empty on the master branch and on unreleased branches.
+#
# WaitEventLWLock and WaitEventLock have their own C code for their wait event
# enums and function names. Hence, for these, only the SGML documentation is
# generated.
@@ -61,6 +67,7 @@ WAL_SENDER_MAIN "Waiting in main loop of WAL sender process."
WAL_SUMMARIZER_WAL "Waiting in WAL summarizer for more WAL to be generated."
WAL_WRITER_MAIN "Waiting in main loop of WAL writer process."
+ABI_compatibility:
#
# Wait Events - Client
@@ -83,6 +90,7 @@ WAIT_FOR_WAL_REPLAY "Waiting for a replay of the particular WAL position on the
WAL_SENDER_WAIT_FOR_WAL "Waiting for WAL to be flushed in WAL sender process."
WAL_SENDER_WRITE_DATA "Waiting for any activity when processing replies from WAL receiver in WAL sender process."
+ABI_compatibility:
#
# Wait Events - IPC
@@ -150,6 +158,7 @@ WAL_RECEIVER_WAIT_START "Waiting for startup process to send initial data for st
WAL_SUMMARY_READY "Waiting for a new WAL summary to be generated."
XACT_GROUP_UPDATE "Waiting for the group leader to update transaction status at end of a parallel operation."
+ABI_compatibility:
#
# Wait Events - Timeout
@@ -170,6 +179,7 @@ VACUUM_DELAY "Waiting in a cost-based vacuum delay point."
VACUUM_TRUNCATE "Waiting to acquire an exclusive lock to truncate off any empty pages at the end of a table vacuumed."
WAL_SUMMARIZER_ERROR "Waiting after a WAL summarizer error."
+ABI_compatibility:
#
# Wait Events - IO
@@ -257,6 +267,7 @@ WAL_SYNC "Waiting for a WAL file to reach durable storage."
WAL_SYNC_METHOD_ASSIGN "Waiting for data to reach durable storage while assigning a new WAL sync method."
WAL_WRITE "Waiting for a write to a WAL file."
+ABI_compatibility:
#
# Wait Events - Buffer Pin
@@ -266,6 +277,7 @@ Section: ClassName - WaitEventBufferPin
BUFFER_PIN "Waiting to acquire an exclusive pin on a buffer."
+ABI_compatibility:
#
# Wait Events - Extension
@@ -275,6 +287,8 @@ Section: ClassName - WaitEventExtension
Extension "Waiting in an extension."
+ABI_compatibility:
+
#
# Wait Events - LWLock
#
@@ -379,6 +393,7 @@ 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."
+# No "ABI_compatibility" region here as has its own C code.
#
# Wait Events - Lock
@@ -401,3 +416,5 @@ object "Waiting to acquire a lock on a non-relation database object."
userlock "Waiting to acquire a user lock."
advisory "Waiting to acquire an advisory user lock."
applytransaction "Waiting to acquire a lock on a remote transaction being applied by a logical replication subscriber."
+
+# No "ABI_compatibility" region here as has its own C code.
--
2.34.1
