Thread: Autogenerate some wait events code and documentation
Hi hackers, In another thread [1], Thomas had the idea to $SUBJECT in a similar way to what is currently done with src/backend/storage/lmgr/lwlocknames.txt. Doing so, like in the attached patch proposal, would help to avoid: - wait event without documentation like observed in [2] - orphaned wait event like observed in [3] The patch relies on a new src/backend/utils/activity/waiteventnames.txt file that contains on row per wait event, with this format: <ENUM NAME> <WAIT EVENT ENUM> <WAIT EVENT NAME> <WAIT EVENT DOC SENTENCE> Then, a new perl script (src/backend/utils/activity/generate-waiteventnames.pl) generates the new: - waiteventnames.c - waiteventnames.h - waiteventnames.sgml files. Remarks: - The new src/backend/utils/activity/waiteventnames.txt file has been created with (a quickly written, non polished and not part of the patch) generate_waiteventnames_txt.sh script attached. Then, the proposal for the 2 wait events missing documentation (non committed yet) done in [2] has been added manually to waiteventnames.txt. - The patch does take care of wait events that currently are linked to enums, means: - PG_WAIT_ACTIVITY - PG_WAIT_CLIENT - PG_WAIT_IPC - PG_WAIT_TIMEOUT - PG_WAIT_IO so that PG_WAIT_LWLOCK, PG_WAIT_LOCK, PG_WAIT_BUFFER_PIN and PG_WAIT_EXTENSION are not autogenerated. This result to having the wait event part of the documentation "monitoring-stats" not ordered as compared to the "Wait EventTypes" Table. This is due to the fact that the new waiteventnames.sgml that contains the documentation for the autogenerated ones listed above is "included" into doc/src/sgml/monitoring.sgml and then breaks the alphabetical ordering with the ones not autogenerated. To fix this I've in mind to also autogenerate enums for PG_WAIT_BUFFER_PIN and PG_WAIT_EXTENSION and split the current documentation "Wait Event Types" Table in 2 tables: one for the autogenerated ones and one (then for PG_WAIT_LWLOCK, PG_WAIT_LOCK) for the non autogenerated "lock" related ones. Looking forward to your feedback, Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com [1]: https://www.postgresql.org/message-id/CA%2BhUKG%2BewEpxm%3DhPNXyupRUB_SKGh-6tO86viaco0g-P_pm_Cw%40mail.gmail.com [2]: https://www.postgresql.org/message-id/CA%2BhUKGJixAHc860Ej9Qzd_z96Z6aoajAgJ18bYfV3Lfn6t9%3D%2BQ%40mail.gmail.com [3]: https://www.postgresql.org/message-id/CA%2BhUKGK6tqm59KuF1z%2Bh5Y8fsWcu5v8%2B84kduSHwRzwjB2aa_A%40mail.gmail.com
Attachment
Hi, On 3/29/23 11:44 AM, Drouvot, Bertrand wrote: > > Looking forward to your feedback, Just realized that more polishing was needed. Done in V2 attached. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Wed, Mar 29, 2023 at 8:51 AM Drouvot, Bertrand <bertranddrouvot.pg@gmail.com> wrote:
Hi,
On 3/29/23 11:44 AM, Drouvot, Bertrand wrote:
>
> Looking forward to your feedback,
Just realized that more polishing was needed.
Done in V2 attached.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
I think this is good work, but I can't help thinking it would be easier to understand and maintain if we used a template engine like Text::Template, and filled out the template with the variant bits. I'll ask that question in another thread for higher visibility.
On Thu, Mar 30, 2023 at 12:41:27PM -0400, Corey Huinker wrote: > I think this is good work, but I can't help thinking it would be easier to > understand and maintain if we used a template engine like Text::Template, > and filled out the template with the variant bits. I'll ask that question > in another thread for higher visibility. Hmm.. This is not part of the main perl distribution, is it? I am not sure that it is a good idea to increase the requirement bar when it comes to build the code and documentation by depending more on external modules, and the minimum version of perl supported is very old^D^D^D ancient, making it harder to satisfy. -- Michael
Attachment
On Wed, Mar 29, 2023 at 02:51:27PM +0200, Drouvot, Bertrand wrote:
> Just realized that more polishing was needed.
>
> Done in V2 attached.
That would be pretty cool to get that done in an automated way, I've
wanted that for a few years now. And I guess that a few others have
the same feeling after missing to update these docs when adding a new
wait event, or just to enforce this alphabetically, so let's do
something about it in v17.
About the alphabetical order, could we have the script enforce a sort
of the elements parsed from waiteventnames.txt, based on the second
column? This now relies on the order of the items in the file, but
my history with this stuff has proved that forcing an ordering rule
would be a very good thing long-term.
Seeing waiteventnames.txt, I think that we should have something
closer to errcodes.txt. Well, seeing the patch, I assume that this is
inspired by errcodes.txt, but this new file should be able to do more
IMO:
- Supporting the parsing of comments, by ignoring them in
generate-waiteventnames.pl.
- Ignore empty likes.
- Add a proper header, copyright, the output generated from it, etc.
- Document the format lines of the file.
It is clear that the format of the file is:
- category
- C symbol in enums.
- Format in the system views.
- Description in the docs.
Or perhaps it would be better to divide this file by sections (like
errcodes.txt) for each category so as we eliminate entirely the first
column?
This number from v2 is nice to see:
17 files changed, 423 insertions(+), 955 deletions(-)
Perhaps waiteventnames.c should be named pgstat_wait_event.c? The
result is simply the set of pgstat functions, included in
wait_event.c (this inclusion is OK for me). Similarly,
wait_event_types.h would be a better name for the set of enums?
utils/adt/jsonpath_scan.c \
+ utils/activity/waiteventnames.c \
+ utils/activity/waiteventnames.h \
+ utils/adt/jsonpath_scan.c \
Looks like a copy-pasto.
Note that the patch does not apply, there is a conflict in the docs.
--
Michael
Attachment
Hi, On 4/20/23 3:09 AM, Michael Paquier wrote: > On Wed, Mar 29, 2023 at 02:51:27PM +0200, Drouvot, Bertrand wrote: >> Just realized that more polishing was needed. >> >> Done in V2 attached. > > That would be pretty cool to get that done in an automated way, I've > wanted that for a few years now. And I guess that a few others have > the same feeling after missing to update these docs when adding a new > wait event, or just to enforce this alphabetically, so let's do > something about it in v17. Thanks for the feedback! > About the alphabetical order, could we have the script enforce a sort > of the elements parsed from waiteventnames.txt, based on the second > column? This now relies on the order of the items in the file, but > my history with this stuff has proved that forcing an ordering rule > would be a very good thing long-term. Not having the lines in order would not have been a problem for the perl script (as it populated the hash table based on the category column while reading the text file). That said I do agree that enforcing an order is a good idea, as it's "easier" to read the generated output files (their content is now somehow "ordered"). This is done in V3 attached. > Seeing waiteventnames.txt, I think that we should have something > closer to errcodes.txt. Well, seeing the patch, I assume that this is > inspired by errcodes.txt, but this new file should be able to do more > IMO: > - Supporting the parsing of comments, by ignoring them in > generate-waiteventnames.pl. > - Ignore empty likes. > - Add a proper header, copyright, the output generated from it, etc. > - Document the format lines of the file. > Fully agree, it's done in V3 attached. > It is clear that the format of the file is: > - category > - C symbol in enums. > - Format in the system views. > - Description in the docs. > Or perhaps it would be better to divide this file by sections (like > errcodes.txt) for each category so as we eliminate entirely the first > column? > Yeah, that could be an option. V3 is still using the category as the first column but I'm ok to change it by a section if you prefer (though I don't really see the need). > Perhaps waiteventnames.c should be named pgstat_wait_event.c? Agree, done. > Similarly, > wait_event_types.h would be a better name for the set of enums? > Also agree, done. > utils/adt/jsonpath_scan.c \ > + utils/activity/waiteventnames.c \ > + utils/activity/waiteventnames.h \ > + utils/adt/jsonpath_scan.c \ > > Looks like a copy-pasto. Why do you think so? both files have to be removed. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Sat, Apr 22, 2023 at 03:36:05PM +0200, Drouvot, Bertrand wrote:
> On 4/20/23 3:09 AM, Michael Paquier wrote:
>> It is clear that the format of the file is:
>> - category
>> - C symbol in enums.
>> - Format in the system views.
>> - Description in the docs.
>> Or perhaps it would be better to divide this file by sections (like
>> errcodes.txt) for each category so as we eliminate entirely the first
>> column?
>
> Yeah, that could be an option. V3 is still using the category as the first column
> but I'm ok to change it by a section if you prefer (though I don't really see the need).
It can make the file width shorter, at least..
[ .. thinks .. ]
+my $waitclass;
+my @wait_classes = ("PG_WAIT_ACTIVITY", "PG_WAIT_CLIENT", "PG_WAIT_IPC", "PG_WAIT_TIMEOUT", "PG_WAIT_IO");
Actually, having a "Section" in waiteventnames.txt would remove the
need to store this knowledge in generate-waiteventnames.pl, which is
a duplicate of the txt contents. If somebody adds a new class in the
future, it would be necessary to update this path as well. Well, that
would not be a huge effort in itself, but IMO the script translating
the .txt to the docs and the code should have no need to know the
types of classes. I guess that a format like that would make the most
sense to me, then:
Section: ClassName PG_WAIT_CLASS_NAME
# ClassName would be "IO", "IPC", "Timeout", etc.
WAIT_EVENT_NAME_1 "WaitEventName1" "Description of wait event 1"
WAIT_EVENT_NAME_N "WaitEventNameN" "Description of wait event N"
>> utils/adt/jsonpath_scan.c \
>> + utils/activity/waiteventnames.c \
>> + utils/activity/waiteventnames.h \
>> + utils/adt/jsonpath_scan.c \
>>
>> Looks like a copy-pasto.
>
> Why do you think so? both files have to be removed.
jsonpath_scan.c is listed twice, and that's still the case in v3. The
list of files deleted for maintainer-clean in src/backend/Makefile
should be listed alphabetically (utils/activity/ before utils/adt/),
but that's a nit ;)
--
Michael
Attachment
Hi,
On 4/24/23 5:15 AM, Michael Paquier wrote:
> On Sat, Apr 22, 2023 at 03:36:05PM +0200, Drouvot, Bertrand wrote:
>> On 4/20/23 3:09 AM, Michael Paquier wrote:
>>> It is clear that the format of the file is:
>>> - category
>>> - C symbol in enums.
>>> - Format in the system views.
>>> - Description in the docs.
>>> Or perhaps it would be better to divide this file by sections (like
>>> errcodes.txt) for each category so as we eliminate entirely the first
>>> column?
>>
>> Yeah, that could be an option. V3 is still using the category as the first column
>> but I'm ok to change it by a section if you prefer (though I don't really see the need).
>
> It can make the file width shorter, at least..
Right.
>
> [ .. thinks .. ]
>
> +my $waitclass;
> +my @wait_classes = ("PG_WAIT_ACTIVITY", "PG_WAIT_CLIENT", "PG_WAIT_IPC", "PG_WAIT_TIMEOUT", "PG_WAIT_IO");
>
> Actually, having a "Section" in waiteventnames.txt would remove the
> need to store this knowledge in generate-waiteventnames.pl, which is
> a duplicate of the txt contents. If somebody adds a new class in the
> future, it would be necessary to update this path as well. Well, that
> would not be a huge effort in itself, but IMO the script translating
> the .txt to the docs and the code should have no need to know the
> types of classes. I guess that a format like that would make the most
> sense to me, then:
> Section: ClassName PG_WAIT_CLASS_NAME
>
> # ClassName would be "IO", "IPC", "Timeout", etc.
>
> WAIT_EVENT_NAME_1 "WaitEventName1" "Description of wait event 1"
> WAIT_EVENT_NAME_N "WaitEventNameN" "Description of wait event N"
>
I gave another thought on it, and do agree that's better to use sections
in the .txt file. This is done in V4 attached.
>>> utils/adt/jsonpath_scan.c \
>>> + utils/activity/waiteventnames.c \
>>> + utils/activity/waiteventnames.h \
>>> + utils/adt/jsonpath_scan.c \
>>>
>>> Looks like a copy-pasto.
>>
>> Why do you think so? both files have to be removed.
>
> jsonpath_scan.c is listed twice, and that's still the case in v3.
Oh I see, I misunderstood what you thought the typo was.
Fixed in V4 thanks!
> The
> list of files deleted for maintainer-clean in src/backend/Makefile
> should be listed alphabetically (utils/activity/ before utils/adt/),
> but that's a nit ;)
Oh right, fixed.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, Apr 24, 2023 at 09:03:53AM +0200, Drouvot, Bertrand wrote:
> Oh right, fixed.
I may tweak a few things if I put my hands on it, but that looks
pretty solid seen from here.. I have spotted a few extra issues.
One thing I have noticed with v4 is that the order of the tables
generated in wait_event_types.h and the SGML docs is inconsistent with
previous versions, and these are not in an alphabetical order. HEAD
orders them as Activity, BufferPin, Client, Extension, IO, IPC, Lock,
LWLock and Timeout. This patch switches the order to become
different, and note that the first table describing each of the wait
event type classes gets it right.
It seems to me that you should apply an extra ordering in
generate-waiteventnames.pl to make sure that the tables are printed in
the same order as previously, around here:
+# Generate the output files
+foreach $waitclass (keys %hashwe) {
(The table describing all the wait event types could be removed from
the SGML docs as well, at the cost of having their description in the
new .txt file. However, as these are long, it would make the .txt
file much messier, so not doing this extra part is OK for me.)
- * Use this category when a process is waiting because it has no work to do,
- * unless the "Client" or "Timeout" category describes the situation better.
- * Typically, this should only be used for background processes
wait_event.h includes a set of comments describing each category, that
this patch removes. Rather than removing this information, which is
helpful to have around, why not making them comments of
waiteventnames.txt instead? Losing this information would be sad.
+# src/backend/utils/activity/pgstat_wait_event.c
+# c functions to get the wait event name based on the enum
Nit. 'c' should be upper-case.
}
+
if (IsNewer(
'src/include/storage/lwlocknames.h',
Not wrong, but this is an unrelated diff.
+if %DIST%==1 if exist src\backend\utils\activity\pgstat_wait_event.c del /q
src\backend\utils\activity\pgstat_wait_event.c
if %DIST%==1 if exist src\backend\storage\lmgr\lwlocknames.h del /q src\backend\storage\lmgr\lwlocknames.h
+if %DIST%==1 if exist src\backend\utils\activity\wait_event_types.h del /q
src\backend\utils\activity\wait_event_types.h
The order here is off a bit. Missed that previously..
perltidy had better be run on generate-waiteventnames.pl (I can do
that myself, no worries), as a couple of lines' format don't seem
quite in line.
--
Michael
Attachment
Hi,
On 4/25/23 7:15 AM, Michael Paquier wrote:
> On Mon, Apr 24, 2023 at 09:03:53AM +0200, Drouvot, Bertrand wrote:
>> Oh right, fixed.
>
> I may tweak a few things if I put my hands on it, but that looks
> pretty solid seen from here..
Glad to hear! ;-)
> I have spotted a few extra issues.
>
> One thing I have noticed with v4 is that the order of the tables
> generated in wait_event_types.h and the SGML docs is inconsistent with
> previous versions, and these are not in an alphabetical order. HEAD
> orders them as Activity, BufferPin, Client, Extension, IO, IPC, Lock,
> LWLock and Timeout. This patch switches the order to become
> different, and note that the first table describing each of the wait
> event type classes gets it right.
>
Right, ordering being somehow broken is also something I did mention initially when I first
presented this patch up-thread. That's also due to the fact that this patch
does not autogenerate PG_WAIT_LWLOCK, PG_WAIT_LOCK, PG_WAIT_BUFFER_PIN and PG_WAIT_EXTENSION.
> It seems to me that you should apply an extra ordering in
> generate-waiteventnames.pl to make sure that the tables are printed in
> the same order as previously, around here:
> +# Generate the output files
> +foreach $waitclass (keys %hashwe) {
>
Yeah but that would still affect only the auto-generated one and then
result to having the wait event part of the documentation "monitoring-stats"
not ordered as compared to the "Wait Event Types" Table.
And as we have only one "include" in doc/src/sgml/monitoring.sgml for all the
auto-generated one, the ordering would still be broken.
> (The table describing all the wait event types could be removed from
> the SGML docs as well, at the cost of having their description in the
> new .txt file. However, as these are long, it would make the .txt
> file much messier, so not doing this extra part is OK for me.)
Right, but that might be a valuable option to also fix the ordering issue
mentioned above (need to look deeper at this).
>
> - * Use this category when a process is waiting because it has no work to do,
> - * unless the "Client" or "Timeout" category describes the situation better.
> - * Typically, this should only be used for background processes
>
> wait_event.h includes a set of comments describing each category, that
> this patch removes. Rather than removing this information, which is
> helpful to have around, why not making them comments of
> waiteventnames.txt instead? Losing this information would be sad.
>
Yeah, good point, I'll look at this.
> +# src/backend/utils/activity/pgstat_wait_event.c
> +# c functions to get the wait event name based on the enum
> Nit. 'c' should be upper-case.
>
> }
> +
> if (IsNewer(
> 'src/include/storage/lwlocknames.h',
> Not wrong, but this is an unrelated diff.
>
Yeah, probably due to a pgindent run.
> +if %DIST%==1 if exist src\backend\utils\activity\pgstat_wait_event.c del /q
src\backend\utils\activity\pgstat_wait_event.c
> if %DIST%==1 if exist src\backend\storage\lmgr\lwlocknames.h del /q src\backend\storage\lmgr\lwlocknames.h
> +if %DIST%==1 if exist src\backend\utils\activity\wait_event_types.h del /q
src\backend\utils\activity\wait_event_types.h
> The order here is off a bit. Missed that previously..
>
> perltidy had better be run on generate-waiteventnames.pl (I can do
> that myself, no worries), as a couple of lines' format don't seem
> quite in line.
Will do, no problem at all.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Hi, On 4/26/23 6:51 PM, Drouvot, Bertrand wrote: > Hi, > > On 4/25/23 7:15 AM, Michael Paquier wrote: > > Will do, no problem at all. > Please find attached V5 addressing the previous comments except the "ordering" one (need to look deeper at this). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Wed, Apr 26, 2023 at 08:36:44PM +0200, Drouvot, Bertrand wrote: > Please find attached V5 addressing the previous comments except > the "ordering" one (need to look deeper at this). I was putting my hands into that, and I see now what you mean here.. Among the nine types of wait events, Lock, LWLock, BufferPin and Extension don't get generated at all. Generating the contents of Lock would mean to gather in a single file the data for the generation of LockTagType in lock.h, the list of LockTagTypeNames in lockfuncs.c and the description of the docs. This data being spread across three files is not really appealing to make that generated.. LWLocks would mean to either extend lwlocknames.txt with the description from the docs if we were to centralize the whole thing. But do we need to merge more data than necessary? We could do things in the simplest fashion possible while making the docs and code user-friendly in the ordering: just add a section for Lock and LWLocks in waiteventnames.txt with an extra comment in their headers and/or data files to tell that waiteventnames.txt also needs a refresh. I would be tempted to do that, actually, and force an ordering for all the wait event categories in generate-waiteventnames.pl with something like that: # Generate the output files -foreach $waitclass (keys %hashwe) +foreach $waitclass (sort keys %hashwe) BufferPin and Extension don't really imply an extra cost by the way: they could just be added to the txt for the wait events even if they have one single element for now. -- Michael
Attachment
Hi, On 4/27/23 8:13 AM, Michael Paquier wrote: > On Wed, Apr 26, 2023 at 08:36:44PM +0200, Drouvot, Bertrand wrote: >> Please find attached V5 addressing the previous comments except >> the "ordering" one (need to look deeper at this). > > I was putting my hands into that, and I see now what you mean here.. > Among the nine types of wait events, Lock, LWLock, BufferPin and > Extension don't get generated at all. > Right. > Generating the contents of Lock would mean to gather in a single file > the data for the generation of LockTagType in lock.h, the list of > LockTagTypeNames in lockfuncs.c and the description of the docs. This > data being spread across three files is not really appealing to make > that generated.. LWLocks would mean to either extend lwlocknames.txt > with the description from the docs if we were to centralize the whole > thing. > > But do we need to merge more data than necessary? We could do things > in the simplest fashion possible while making the docs and code > user-friendly in the ordering: just add a section for Lock and LWLocks > in waiteventnames.txt with an extra comment in their headers and/or > data files to tell that waiteventnames.txt also needs a refresh. Agree that it would fix the doc ordering and that we could do that. It's done that way in V6. There is already comments about this in lockfuncs.c and lwlocknames.txt, so V6 updates those comments accordingly. > I would be tempted to do that, actually, and force an ordering for all > the wait event categories in generate-waiteventnames.pl with something > like that: > # Generate the output files > -foreach $waitclass (keys %hashwe) > +foreach $waitclass (sort keys %hashwe) > Agree, done in V6. > BufferPin and Extension don't really imply an extra cost by the way: > they could just be added to the txt for the wait events even if they > have one single element for now. Right, done that way in V6. Please note that it creates 2 new "wait events": WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN. Then, they replace PG_WAIT_EXTENSION and PG_WAIT_BUFFER_PIN (resp.) where appropriate. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Apr 28, 2023 at 02:29:13PM +0200, Drouvot, Bertrand wrote:
> On 4/27/23 8:13 AM, Michael Paquier wrote:
>> Generating the contents of Lock would mean to gather in a single file
>> the data for the generation of LockTagType in lock.h, the list of
>> LockTagTypeNames in lockfuncs.c and the description of the docs. This
>> data being spread across three files is not really appealing to make
>> that generated.. LWLocks would mean to either extend lwlocknames.txt
>> with the description from the docs if we were to centralize the whole
>> thing.
>>
>> But do we need to merge more data than necessary? We could do things
>> in the simplest fashion possible while making the docs and code
>> user-friendly in the ordering: just add a section for Lock and LWLocks
>> in waiteventnames.txt with an extra comment in their headers and/or
>> data files to tell that waiteventnames.txt also needs a refresh.
>
> Agree that it would fix the doc ordering and that we could do that.
Not much a fan of the part where a full paragraph of the SGML docs is
added to the .txt, particularly with the new handling for "Notes".
I'd rather shape the perl script to be minimalistic and simpler, even
if it means moving this paragraph about LWLocks after all the tables
are generated.
Do we also need the comments in the generated header as well? My
initial impression was to just move these as comments of the .txt file
because that's where the new events would be added, as the .txt is the
main point of reference.
> It's done that way in V6.
>
> There is already comments about this in lockfuncs.c and lwlocknames.txt, so
> V6 updates those comments accordingly.
>
> Right, done that way in V6.
>
> Please note that it creates 2 new "wait events":
> WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN.
Noted. Makes sense here.
> Then, they replace PG_WAIT_EXTENSION and PG_WAIT_BUFFER_PIN (resp.) where appropriate.
So, the change here..
+ # Exception here
+ if ($last =~ /^BufferPin/)
+ {
+ $last = "Buffer_Pin";
+ }
.. Implies the two following changes:
typedef enum
{
- WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFER_PIN
+ WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFERPIN
} WaitEventBufferPin;
[...]
static const char *
-pgstat_get_wait_buffer_pin(WaitEventBufferPin w)
+pgstat_get_wait_bufferpin(WaitEventBufferPin w)
I would be OK to remove this exception in the script as it does not
change anything for the end user (the wait event string is still
reported as "BufferPin"). This way, we keep things simpler in the
script. This has as extra consequence to require a change in
wait_event.h so as PG_WAIT_BUFFER_PIN is renamed to PG_WAIT_BUFFERPIN,
equally fine by me. Logically, this rename should be done in a patch
of its own, for clarity.
@@ -185,6 +193,7 @@ distprep:
$(MAKE) -C utils distprep
$(MAKE) -C utils/adt jsonpath_gram.c jsonpath_gram.h jsonpath_scan.c
$(MAKE) -C utils/misc guc-file.c
+ $(MAKE) -C utils/actvity wait_event_types.h pgstat_wait_event.c
Incorrect order, and incorrect name (s/actvity/activity/, lacking an
'i').
+printf $h $header_comment, 'wait_event_types.h';
+printf $h "#ifndef WAITEVENTNAMES_H\n";
+printf $h "#define WAITEVENTNAMES_H\n\n";
Inconsistency detected here.
It seems to me that we'd better have a .gitignore in utils/activity/
for the new files.
@@ -237,7 +237,7 @@ autoprewarm_main(Datum main_arg)
(void) WaitLatch(MyLatch,
WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
-1L,
- PG_WAIT_EXTENSION);
+ WAIT_EVENT_EXTENSION);
Perhaps this should also be part of a first, separate patch, with the
introduction of the new pgstat_get_wait_extension/bufferpin()
functions. Okay, it is not a big deal because the main patch
generates the enum for extensions which would be used here, but for
the sake of history clarity I'd rather refactor and rename all that
first.
The choices of LWLOCK and LOCK for the internal names was a bit
surprising, while we can be consistent with the rest and use "LWLock"
and "Lock".
Attached is a v7 with the portions I have adjusted, which is mostly OK
by me at this point. We are still away from the next CF, but I'll
look at that again when the v17 branch opens.
--
Michael
Attachment
> [patch] This is not a review of the perl/make/meson glue/details, but I just wanted to say thanks for working on this Bertrand & Michael, at a quick glance that .txt file looks like it's going to be a lot more fun to maintain!
Hi,
On 5/1/23 1:59 AM, Michael Paquier wrote:
> On Fri, Apr 28, 2023 at 02:29:13PM +0200, Drouvot, Bertrand wrote:
>> On 4/27/23 8:13 AM, Michael Paquier wrote:
>>>
>>> But do we need to merge more data than necessary? We could do things
>>> in the simplest fashion possible while making the docs and code
>>> user-friendly in the ordering: just add a section for Lock and LWLocks
>>> in waiteventnames.txt with an extra comment in their headers and/or
>>> data files to tell that waiteventnames.txt also needs a refresh.
>>
>> Agree that it would fix the doc ordering and that we could do that.
>
> Not much a fan of the part where a full paragraph of the SGML docs is
> added to the .txt, particularly with the new handling for "Notes".
I understand your concern.
> I'd rather shape the perl script to be minimalistic and simpler, even
> if it means moving this paragraph about LWLocks after all the tables
> are generated.
I'm not sure I like it. First, it does break the "Note" ordering as compare
to the current documentation. That's not a big deal though.
Secondly, what If we need to add some note(s) in the future for another wait class? Having all the notes
after all the tables are generated would sound weird to me.
We could discuss another approach for the "Note" part if there is a need to add one for an existing/new wait class
though.
>
> Do we also need the comments in the generated header as well? My
> initial impression was to just move these as comments of the .txt file
> because that's where the new events would be added, as the .txt is the
> main point of reference.
>
Oh I see. The advantage of the previous approach is to have them at both places (.txt and header).
But that said I understand your point about having the perl script minimalistic and simpler.
>> Please note that it creates 2 new "wait events":
>> WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN.
>
> Noted. Makes sense here.
Yup and that may help to add "custom" wait event for extensions too (need to think about it once
this refactoring is done).
> So, the change here..
> + # Exception here
> + if ($last =~ /^BufferPin/)
> + {
> + $last = "Buffer_Pin";
> + }
>
> .. Implies the two following changes:
> typedef enum
> {
> - WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFER_PIN
> + WAIT_EVENT_BUFFER_PIN = PG_WAIT_BUFFERPIN
> } WaitEventBufferPin;
> [...]
> static const char *
> -pgstat_get_wait_buffer_pin(WaitEventBufferPin w)
> +pgstat_get_wait_bufferpin(WaitEventBufferPin w)
>
> I would be OK to remove this exception in the script as it does not
> change anything for the end user (the wait event string is still
> reported as "BufferPin"). This way, we keep things simpler in the
> script.
Good point, agree.
> This has as extra consequence to require a change in
> wait_event.h so as PG_WAIT_BUFFER_PIN is renamed to PG_WAIT_BUFFERPIN,
> equally fine by me. Logically, this rename should be done in a patch
> of its own, for clarity.
Yes, I can look at it.
>
> @@ -185,6 +193,7 @@ distprep:
> $(MAKE) -C utils distprep
> $(MAKE) -C utils/adt jsonpath_gram.c jsonpath_gram.h jsonpath_scan.c
> $(MAKE) -C utils/misc guc-file.c
> + $(MAKE) -C utils/actvity wait_event_types.h pgstat_wait_event.c
> Incorrect order, and incorrect name (s/actvity/activity/, lacking an
> 'i').
>
Nice catch.
> +printf $h $header_comment, 'wait_event_types.h';
> +printf $h "#ifndef WAITEVENTNAMES_H\n";
> +printf $h "#define WAITEVENTNAMES_H\n\n";
> Inconsistency detected here.
>
> It seems to me that we'd better have a .gitignore in utils/activity/
> for the new files.
>
Agree.
> @@ -237,7 +237,7 @@ autoprewarm_main(Datum main_arg)
> (void) WaitLatch(MyLatch,
> WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
> -1L,
> - PG_WAIT_EXTENSION);
> + WAIT_EVENT_EXTENSION);
>
> Perhaps this should also be part of a first, separate patch, with the
> introduction of the new pgstat_get_wait_extension/bufferpin()
> functions. Okay, it is not a big deal because the main patch
> generates the enum for extensions which would be used here, but for
> the sake of history clarity I'd rather refactor and rename all that
> first.
>
Agree, I'll look at this.
> The choices of LWLOCK and LOCK for the internal names was a bit
> surprising, while we can be consistent with the rest and use "LWLock"
> and "Lock".
>
> Attached is a v7 with the portions I have adjusted, which is mostly OK
> by me at this point. We are still away from the next CF, but I'll
> look at that again when the v17 branch opens.
Thanks for the v7! I did not look at the details but just replied to this thread.
I'll look at v7 when the v17 branch opens and propose the separate patch
mentioned above at that time too.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Hi, On 5/2/23 4:50 AM, Thomas Munro wrote: >> [patch] > > This is not a review of the perl/make/meson glue/details, but I just > wanted to say thanks for working on this Bertrand & Michael, at a > quick glance that .txt file looks like it's going to be a lot more fun > to maintain! Thanks Thomas! Yeah and probably less error prone too ;-) Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, May 04, 2023 at 08:39:49AM +0200, Drouvot, Bertrand wrote: > On 5/1/23 1:59 AM, Michael Paquier wrote: > I'm not sure I like it. First, it does break the "Note" ordering as compare > to the current documentation. That's not a big deal though. > > Secondly, what If we need to add some note(s) in the future for > another wait class? Having all the notes after all the tables are > generated would sound weird to me. Appending these notes at the end of all the tables does not strike me as a big dea, TBH. But, well, my sole opinion is not the final choice either. For now, I am mostly tempted to keep the generation script as minimalistic as possible. > We could discuss another approach for the "Note" part if there is a > need to add one for an existing/new wait class though. Documenting what's expected from the wait event classes is critical in the .txt file as that's what developers are going to look at when adding a new wait event. Adding them in the header is less appealing to me considering that is it now generated, and the docs provide a lot of explanation as well. >> This has as extra consequence to require a change in >> wait_event.h so as PG_WAIT_BUFFER_PIN is renamed to PG_WAIT_BUFFERPIN, >> equally fine by me. Logically, this rename should be done in a patch >> of its own, for clarity. > > Yes, I can look at it. > [...] > Agree, I'll look at this. Thanks! > I'll look at v7 when the v17 branch opens and propose the separate patch > mentioned above at that time too. Thanks, again. -- Michael
Attachment
On Sat, May 06, 2023 at 11:23:17AM +0900, Michael Paquier wrote: >> I'll look at v7 when the v17 branch opens and propose the separate patch >> mentioned above at that time too. > > Thanks, again. By the way, while browsing through the patch, I have noticed two things: - The ordering of the items for Lock and LWLock is incorrect. - We are missing some of the LWLock entries, like CommitTsBuffer, XactBuffer or WALInsert, as of all the entries in BuiltinTrancheNames. My apologies for not noticing that earlier. This exists in v6 as much as v7. -- Michael
Attachment
Hi, On 5/6/23 4:23 AM, Michael Paquier wrote: > On Thu, May 04, 2023 at 08:39:49AM +0200, Drouvot, Bertrand wrote: >> On 5/1/23 1:59 AM, Michael Paquier wrote: >> I'm not sure I like it. First, it does break the "Note" ordering as compare >> to the current documentation. That's not a big deal though. >> >> Secondly, what If we need to add some note(s) in the future for >> another wait class? Having all the notes after all the tables are >> generated would sound weird to me. > > Appending these notes at the end of all the tables does not strike me > as a big dea, TBH. But, well, my sole opinion is not the final choice > either. For now, I am mostly tempted to keep the generation script as > minimalistic as possible. > I agree that's not a big deal and I'm not against having these notes at the end of all the tables. >> We could discuss another approach for the "Note" part if there is a >> need to add one for an existing/new wait class though. > means, that was more a NIT comment from my side. > Documenting what's expected from the wait event classes is critical in > the .txt file as that's what developers are going to look at when > adding a new wait event. Adding them in the header is less appealing > to me considering that is it now generated, and the docs provide a lot > of explanation as well. > Your argument that the header is now generated makes me change my mind: I know think that having the comments in the .txt file is enough. >>> This has as extra consequence to require a change in >>> wait_event.h so as PG_WAIT_BUFFER_PIN is renamed to PG_WAIT_BUFFERPIN, >>> equally fine by me. Logically, this rename should be done in a patch >>> of its own, for clarity. >> >> Yes, I can look at it. >> [...] >> Agree, I'll look at this. > > Thanks! Please find the dedicated patch proposal in [1]. [1]: https://www.postgresql.org/message-id/c6f35117-4b20-4c78-1df5-d3056010dcf5%40gmail.com Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 5/6/23 4:37 AM, Michael Paquier wrote: > On Sat, May 06, 2023 at 11:23:17AM +0900, Michael Paquier wrote: >>> I'll look at v7 when the v17 branch opens and propose the separate patch >>> mentioned above at that time too. >> >> Thanks, again. > > By the way, while browsing through the patch, I have noticed two > things: > - The ordering of the items for Lock and LWLock is incorrect. Oh right, fixed in V8 attached (moved the sort on the third column instead of the second which has always the same content "WAIT_EVENT_DOCONLY" for Lock and LWLock). > - We are missing some of the LWLock entries, like CommitTsBuffer, > XactBuffer or WALInsert, as of all the entries in > BuiltinTrancheNames. > Yeah, my bad. Fixed in V8 attached. > My apologies for not noticing that earlier. This exists in v6 as much > as v7. No problem at all and thanks for the call out! Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, May 15, 2023 at 06:45:23PM +0200, Drouvot, Bertrand wrote:
> On 5/6/23 4:37 AM, Michael Paquier wrote:
>> On Sat, May 06, 2023 at 11:23:17AM +0900, Michael Paquier wrote:
>>>> I'll look at v7 when the v17 branch opens and propose the separate patch
>>>> mentioned above at that time too.
>>>
>>> Thanks, again.
>>
>> By the way, while browsing through the patch, I have noticed two
>> things:
>> - The ordering of the items for Lock and LWLock is incorrect.
>
> Oh right, fixed in V8 attached (moved the sort on the third column
> instead of the second which has always the same content "WAIT_EVENT_DOCONLY"
> for Lock and LWLock).
Ah, I didn't notice that. Makes sense.
>> - We are missing some of the LWLock entries, like CommitTsBuffer,
>> XactBuffer or WALInsert, as of all the entries in
>> BuiltinTrancheNames.
>
> Yeah, my bad. Fixed in V8 attached.
BufFileTruncate and BufFileWrite have an incorrect order in HEAD's
monitoring.sgml (will fix in a minute for 16~). So your patch fixes
that.
PgStatsDSA and PgStatsData are reversed in your patch compared to
HEAD, actually, based on the way perl sees fit to do its ordering by
giving priority to upper-case characters. Same for RelCacheInit and
RelationMapping, or even SInvalRead/SInvalWrite being now before the
"Serial" family. Worse, the tables LWLock and Lock are in an
incorrect order as well with the patch. We'd better be a bit more
verbose with the sort step, I think.. perl does not handle well
sorting with collations from what I recall, but we could use uc() with
a block sort to force the operation to be case-insensitive, like "sort
{uc($a) cmp uc($b)}". That needs to be applied here, I guess:
+# Sort the lines based on the third column
+my @lines_sorted =
+ sort { (split(/\t/, $a))[2] cmp(split(/\t/, $b))[2] } @lines;
And it looks like you'd need to apply uc() on each [2] element. I
would add a comment about this detail, as well.
No entries are missing, after comparing what's generated by the patch
with the contents of HEAD.
Small nit-ish question: waiteventnames.sgml or wait_event_types.sgml?
Same for generate-waiteventtypes.pl?
>> My apologies for not noticing that earlier. This exists in v6 as much
>> as v7.
>
> No problem at all and thanks for the call out!
FWIW, I would have posted two patches, one with the refactoring of
done in [1], and a second that switches to the automation, to make
clear the preparatory step.
[1]: https://www.postgresql.org/message-id/c6f35117-4b20-4c78-1df5-d3056010dcf5@gmail.com
--
Michael
Attachment
Hi,
On 5/16/23 9:48 AM, Michael Paquier wrote:
> On Mon, May 15, 2023 at 06:45:23PM +0200, Drouvot, Bertrand wrote:
>> On 5/6/23 4:37 AM, Michael Paquier wrote:
>>> On Sat, May 06, 2023 at 11:23:17AM +0900, Michael Paquier wrote:
>
> BufFileTruncate and BufFileWrite have an incorrect order in HEAD's
> monitoring.sgml (will fix in a minute for 16~). So your patch fixes
> that.
Oh nice catch!
>
> PgStatsDSA and PgStatsData are reversed in your patch compared to
> HEAD, actually, based on the way perl sees fit to do its ordering by
> giving priority to upper-case characters. Same for RelCacheInit and
> RelationMapping, or even SInvalRead/SInvalWrite being now before the
> "Serial" family. Worse, the tables LWLock and Lock are in an
> incorrect order as well with the patch. We'd better be a bit more
> verbose with the sort step, I think.. perl does not handle well
> sorting with collations from what I recall, but we could use uc() with
> a block sort to force the operation to be case-insensitive, like "sort
> {uc($a) cmp uc($b)}". That needs to be applied here, I guess:
> +# Sort the lines based on the third column
> +my @lines_sorted =
> + sort { (split(/\t/, $a))[2] cmp(split(/\t/, $b))[2] } @lines;
>
Oh right, nice catch.
> And it looks like you'd need to apply uc() on each [2] element. I
> would add a comment about this detail, as well.
>
Did it that way in V9 attached and the sorting does look like what we expect now.
> No entries are missing, after comparing what's generated by the patch
> with the contents of HEAD.
>
> Small nit-ish question: waiteventnames.sgml or wait_event_types.sgml?
> Same for generate-waiteventtypes.pl?
>
Agree, it's more consistent. Done that way in V9.
>
> FWIW, I would have posted two patches, one with the refactoring of
> done in [1], and a second that switches to the automation, to make
> clear the preparatory step.
>
> [1]: https://www.postgresql.org/message-id/c6f35117-4b20-4c78-1df5-d3056010dcf5@gmail.com
> --
Agree, V9 does now apply on top of v2-0001-Introducing-WAIT_EVENT_EXTENSION-and-WAIT_EVENT_B.patch
(just shared in [1]).
[1]: https://www.postgresql.org/message-id/a82c2660-64b4-1c59-3eef-bf82b86fb99a%40gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachment
On Wed, May 17, 2023 at 08:31:53AM +0200, Drouvot, Bertrand wrote: > Did it that way in V9 attached and the sorting does look like what > we expect now. Yes, the order of the items in the individual tables is fine, but this is still a bit incorrect for the classes? Note that the tables for the LWLock and Lock are still in reverse order :) +foreach $waitclass (sort keys %hashwe) Meaning that you may want to add an extra case-insensitive rule for the sorting on this line for the SGML docs (also the C part, I guess, but we care less). > Agree, V9 does now apply on top of v2-0001-Introducing-WAIT_EVENT_EXTENSION-and-WAIT_EVENT_B.patch > (just shared in [1]). If you don't send both patches in the same message the CF bot is going to complain as v9-0001 is not able to apply independently of the other patch v2-0001 on the other thread (you could do a git apply -2 -v2, for example). -- Michael
Attachment
Hi, On 5/17/23 10:14 AM, Michael Paquier wrote: > On Wed, May 17, 2023 at 08:31:53AM +0200, Drouvot, Bertrand wrote: >> Did it that way in V9 attached and the sorting does look like what >> we expect now. > > Yes, the order of the items in the individual tables is fine, but this > is still a bit incorrect for the classes? Note that the tables for > the LWLock and Lock are still in reverse order :) Sorry did not pay enough attention to it ;-( > +foreach $waitclass (sort keys %hashwe) > > Meaning that you may want to add an extra case-insensitive rule for > the sorting on this line for the SGML docs (also the C part, I guess, > but we care less). Yeap, done in V10 for sgml and the C part too (for consistency). >> Agree, V9 does now apply on top of v2-0001-Introducing-WAIT_EVENT_EXTENSION-and-WAIT_EVENT_B.patch >> (just shared in [1]). > > If you don't send both patches in the same message the CF bot is going > to complain as v9-0001 is not able to apply independently of the other > patch v2-0001 on the other thread Yeah, good point, attaching both here to keep the CF bot happy. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Wed, May 17, 2023 at 11:10:21AM +0200, Drouvot, Bertrand wrote: > Sorry did not pay enough attention to it ;-( No problem. > Yeap, done in V10 for sgml and the C part too (for consistency). The order looks fine seen from here, thanks! -- Michael
Attachment
On Thu, May 18, 2023 at 01:36:30PM +0900, Michael Paquier wrote: > The order looks fine seen from here, thanks! Now that v17 is open for business, I have looked again at this patch. perlcritic is formulating three complaints: ./src/backend/utils/activity/generate-waiteventtypes.pl: Loop iterator is not lexical at line 99, column 1. See page 108 of PBP. ([Variables::RequireLexicalLoopIterators] Severity: 5) ./src/backend/utils/activity/generate-waiteventtypes.pl: Loop iterator is not lexical at line 126, column 1. See page 108 of PBP. ([Variables::RequireLexicalLoopIterators] Severity: 5) ./src/backend/utils/activity/generate-waiteventtypes.pl: Loop iterator is not lexical at line 181, column 1. See page 108 of PBP. ([Variables::RequireLexicalLoopIterators] Severity: 5) These are caused by three foreach loops, where perl wants to use a local declaration for the iterators. The indentation was a bit off, as well, perltidy v20230309 has reported a few diffs. Not a big deal. src/common/meson.build includes the following comment: # For the server build of pgcommon, depend on lwlocknames_h, because at least # cryptohash_openssl.c, hmac_openssl.c depend on it. That's arguably a # layering violation, but ... The thing is that controldata_utils.c has a dependency to wait events so we should add wait_event_types_h to 'sources'. The names chosen, as of wait_event_types.h, pgstat_wait_event.c, waiteventnames.txt and generate-wait_event_types.pl are inconsistent, comparing them for instance with the lwlock parts. I have renamed these a bit, with more underscores. Building the documentation in a meson/ninja build can be done with the following command run from the root of the build directory: ninja alldocs However this command fails with v10. The step that fails is: [6/14] Generating doc/src/sgml/postgres-full.xml with a custom command It seems to me that the correct thing to do is to add --outdir @OUTDIR@ to the command? However, I do see a few problems even after that: - The C and H files are still generated in doc/src/sgml/, which is useless. - The SGML file wait_event_types.sgml in doc/src/sgml/ seems to be empty, still to my surprise the HTML part was created correctly. - The SGML file is not needed for the C code. I think that we should add some options to the perl script to be more selective with the files generated. How about having two options called --docs and --code to select one or the other, then limit what gets generated in each path? I guess that it would be cleaner if we error in the case where both options are defined, and just use some gotos to redirect to each foreach loop to limit extra indentations in the script. This would avoid the need to remove the C and H files from the docs, additionally, which is what the Makefile in doc/ does. I have fixed all the issues I've found in v11 attached, except for the last one (I have added the OUTDIR trick for reference, but that's incorrect and incomplete). Could you look at this part? -- Michael
Attachment
On Mon, Jul 03, 2023 at 03:57:42PM +0900, Michael Paquier wrote: > I think that we should add some options to the perl script to be more > selective with the files generated. How about having two options > called --docs and --code to select one or the other, then limit what > gets generated in each path? I guess that it would be cleaner if we > error in the case where both options are defined, and just use some > gotos to redirect to each foreach loop to limit extra indentations in > the script. This would avoid the need to remove the C and H files > from the docs, additionally, which is what the Makefile in doc/ does. > > I have fixed all the issues I've found in v11 attached, except for the > last one (I have added the OUTDIR trick for reference, but that's > incorrect and incomplete). Could you look at this part? Ah. It took me a few extra minutes, but I think that we should set "capture" to "false", no? It looks like meson is getting confused, expecting something in stdout but the new script generates a few files, and does not output anything. That's different from the other doc-related perl scripts. -- Michael
Attachment
Hi, On 7/3/23 9:11 AM, Michael Paquier wrote: > On Mon, Jul 03, 2023 at 03:57:42PM +0900, Michael Paquier wrote: Thanks for looking at it and having fixed the issues that were present in v10. >> I think that we should add some options to the perl script to be more >> selective with the files generated. How about having two options >> called --docs and --code to select one or the other, then limit what >> gets generated in each path? I guess that it would be cleaner if we >> error in the case where both options are defined, and just use some >> gotos to redirect to each foreach loop to limit extra indentations in >> the script. This would avoid the need to remove the C and H files >> from the docs, additionally, which is what the Makefile in doc/ does. >> >> I have fixed all the issues I've found in v11 attached, except for the >> last one (I have added the OUTDIR trick for reference, but that's >> incorrect and incomplete). Could you look at this part? > > Ah. It took me a few extra minutes, but I think that we should set > "capture" to "false", no? It looks like meson is getting confused, > expecting something in stdout but the new script generates a few > files, and does not output anything. That's different from the other > doc-related perl scripts. > -- Yeah, with "capture" set to "false" then ninja alldocs does not error out and wait_event_types.sgml gets generated. I'll look at the extra options --code and --docs. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 7/4/23 9:34 AM, Drouvot, Bertrand wrote: > Hi, > > On 7/3/23 9:11 AM, Michael Paquier wrote: >> On Mon, Jul 03, 2023 at 03:57:42PM +0900, Michael Paquier wrote: > > Thanks for looking at it and having fixed the issues that were present in > v10. > >>> I think that we should add some options to the perl script to be more >>> selective with the files generated. How about having two options >>> called --docs and --code to select one or the other, then limit what >>> gets generated in each path? I guess that it would be cleaner if we >>> error in the case where both options are defined, and just use some >>> gotos to redirect to each foreach loop to limit extra indentations in >>> the script. This would avoid the need to remove the C and H files >>> from the docs, additionally, which is what the Makefile in doc/ does. >>> >>> I have fixed all the issues I've found in v11 attached, except for the >>> last one (I have added the OUTDIR trick for reference, but that's >>> incorrect and incomplete). Could you look at this part? >> >> Ah. It took me a few extra minutes, but I think that we should set >> "capture" to "false", no? It looks like meson is getting confused, >> expecting something in stdout but the new script generates a few >> files, and does not output anything. That's different from the other >> doc-related perl scripts. >> -- > > Yeah, with "capture" set to "false" then ninja alldocs does not error out > and wait_event_types.sgml gets generated. > > I'll look at the extra options --code and --docs. Please find attached v12 that: - sets "capture" to "false" - adds the --code and --docs extra options to generate-wait_event_types.pl - makes sure at least one of those option is provided - makes sure that both options can't be provided simultaneously - update the related Makefile/meson.build files accordingly - fix a bug in generate-wait_event_types.pl (die on rename($stmp...) was not using the right file (it was using ctmp). That was not visible before the docs/code split. Not related to this patch but I noticed that when building with meson some c files are duplicated a the end of the build. Indeed, they also appear in some include directories: ./meson_build/src/include/storage/lwlocknames.c ./meson_build/src/include/utils/pgstat_wait_event.c ./meson_build/src/include/utils/fmgrtab.c ./meson_build/src/include/nodes/queryjumblefuncs.funcs.c ./meson_build/src/include/nodes/readfuncs.switch.c ./meson_build/src/include/nodes/readfuncs.funcs.c ./meson_build/src/include/nodes/copyfuncs.switch.c ./meson_build/src/include/nodes/equalfuncs.funcs.c ./meson_build/src/include/nodes/outfuncs.switch.c ./meson_build/src/include/nodes/queryjumblefuncs.switch.c ./meson_build/src/include/nodes/copyfuncs.funcs.c ./meson_build/src/include/nodes/equalfuncs.switch.c ./meson_build/src/include/nodes/outfuncs.funcs.c Is it expected? If not, I guess it's worth another patch. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, Jul 04, 2023 at 09:34:33AM +0200, Drouvot, Bertrand wrote:
> Yeah, with "capture" set to "false" then ninja alldocs does not error out
> and wait_event_types.sgml gets generated.
>
> I'll look at the extra options --code and --docs.
+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 $< > $@
This is doing the same error as meson in v10, there is no need for
the last part doing the redirection because the script outputs
nothing. Here is the command generated:
make -C doc/src/sgml/ wait_event_types.sgml
'/usr/bin/perl'
../../../src/backend/utils/activity/generate-wait_event_types.pl
--docs ../../../src/backend/utils/activity/wait_event_names.txt >
wait_event_types.sgml
+wait_event_names = custom_target('wait_event_names',
+ input: files('../../backend/utils/activity/wait_event_names.txt'),
+ output: ['wait_event_types.h'],
This one was not completely correct (look at fmgrtab, for example), as
it is missing pgstat_wait_event.c in the output generated. We could
perhaps be more selective with all that, including fmgrtab, but I have
left that out for now. Note also the tweak with install_dir to not
install the C file.
+wait_event_names = custom_target('wait_event_names',
+ input: files('./wait_event_names.txt'),
+ output: ['pgstat_wait_event.c'],
+ command: [
+ perl, files('./generate-wait_event_types.pl'),
+ '-o', '@OUTDIR@', '--code',
+ '@INPUT@'
+ ],
+ install: true,
+ install_dir: [false],
+)
[...]
+# these include .c files generated in ../../../include/activity, seems nicer to not
+# add that as an include path for the whole backend
+waitevent_sources = files(
'wait_event.c',
)
+
+backend_link_with += static_library('wait_event_names',
+ waitevent_sources,
+ dependencies: [backend_code],
+ include_directories: include_directories('../../../include/utils'),
+ kwargs: internal_lib_args,
+)
"wait_event_names" with the extra command should not be necessary
here, because we feed from the C file generated in src/include/utils/,
included in wait_event.c. See src/backend/nodes/meson.build for a
similar example
Two of the error messages after rename() in the script were
inconsistent. So reworded these on the way.
I have added a usage() to the script, while on it.
The VPATH build was broken, because the following line was missing
from src/backend/utils/activity/Makefile to be able to detect
pgstat_wait_event.c from wait_event.c:
override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
With all that in place, VPATH builds, the CI, meson, configure/make
and the various cleanup targets were working fine, so I have applied
it. Now let's see what the buildfarm tells.
The final --stat number is like that:
22 files changed, 757 insertions(+), 2111 deletions(-)
--
Michael
Attachment
Hi,
On 2023-07-05 10:57:19 +0900, Michael Paquier wrote:
> With all that in place, VPATH builds, the CI, meson, configure/make
> and the various cleanup targets were working fine, so I have applied
> it. Now let's see what the buildfarm tells.
>
> The final --stat number is like that:
> 22 files changed, 757 insertions(+), 2111 deletions(-)
That's pretty nice!
Rebasing a patch over this I was a bit confused because I got a bunch of
""unable to parse wait_event_names.txt" errors. Took me a while to figure out
that that was just because I didn't include a trailing . in the description.
Perhaps that could be turned into a more meaningful error?
die "unable to parse wait_event_names.txt"
unless $line =~ /^(\w+)\t+(\w+)\t+("\w+")\t+("\w.*\.")$/;
It's not helped by the fact that the regex in the error actually doesn't match
any lines, because it's not operating on the input but on
push(@lines, $section_name . "\t" . $_);
I also do wonder if we should invest in generating the lwlock names as
well. Except for a few abbreviations, the second column is always the
camel-cased version of what follows WAIT_EVENT_. Feels pretty tedious to write
that out.
Perhaps we should just change the case of the upper-cased names (DSM, SSL,
WAL, ...) to follow the other names?
Greetings,
Andres Freund
On Wed, Jul 05, 2023 at 02:59:39PM -0700, Andres Freund wrote:
> Rebasing a patch over this I was a bit confused because I got a bunch of
> ""unable to parse wait_event_names.txt" errors. Took me a while to figure out
> that that was just because I didn't include a trailing . in the description.
> Perhaps that could be turned into a more meaningful error?
>
> die "unable to parse wait_event_names.txt"
> unless $line =~ /^(\w+)\t+(\w+)\t+("\w+")\t+("\w.*\.")$/;
Agreed that we could at least add the $line in the error message
generated, at least, to help with debugging.
> I also do wonder if we should invest in generating the lwlock names as
> well. Except for a few abbreviations, the second column is always the
> camel-cased version of what follows WAIT_EVENT_. Feels pretty tedious to write
> that out.
And you mean getting rid of lwlocknames.txt? The impact on dtrace or
other similar tools is uncertain to me because we have free number on
this list, and stuff like GetLWLockIdentifier() rely on the input ID
from lwlocknames.txt.
> Perhaps we should just change the case of the upper-cased names (DSM, SSL,
> WAL, ...) to follow the other names?
So you mean renaming the existing events like WalSenderWaitForWAL to
WalSenderWaitForWal? The impact on existing monitoring queries is not
zero because any changes would be silent, and that's the part that
worried me the most even if it can remove one column in the txt file.
--
Michael
Attachment
Hi,
On 2023-07-06 09:36:12 +0900, Michael Paquier wrote:
> On Wed, Jul 05, 2023 at 02:59:39PM -0700, Andres Freund wrote:
> > Rebasing a patch over this I was a bit confused because I got a bunch of
> > ""unable to parse wait_event_names.txt" errors. Took me a while to figure out
> > that that was just because I didn't include a trailing . in the description.
> > Perhaps that could be turned into a more meaningful error?
> >
> > die "unable to parse wait_event_names.txt"
> > unless $line =~ /^(\w+)\t+(\w+)\t+("\w+")\t+("\w.*\.")$/;
>
> Agreed that we could at least add the $line in the error message
> generated, at least, to help with debugging.
>
> > I also do wonder if we should invest in generating the lwlock names as
> > well. Except for a few abbreviations, the second column is always the
> > camel-cased version of what follows WAIT_EVENT_. Feels pretty tedious to write
> > that out.
>
> And you mean getting rid of lwlocknames.txt?
No, I meant the second column in wait_event_names.txt. If you look at stuff
like:
WAIT_EVENT_ARCHIVER_MAIN "ArchiverMain" "Waiting in main loop of archiver process."
It'd be pretty trivial to generate ArchiverMain from ARCHIVER_MAIN.
> The impact on dtrace or other similar tools is uncertain to me because we
> have free number on this list, and stuff like GetLWLockIdentifier() rely on
> the input ID from lwlocknames.txt.
I don't really care, tbh. If we wanted to keep the names the same in case of
abbreviations, we could just make the name optional, and auto-generated if not
explicitly specified.
> > Perhaps we should just change the case of the upper-cased names (DSM, SSL,
> > WAL, ...) to follow the other names?
>
> So you mean renaming the existing events like WalSenderWaitForWAL to
> WalSenderWaitForWal?
Yes.
> The impact on existing monitoring queries is not zero because any changes
> would be silent, and that's the part that worried me the most even if it can
> remove one column in the txt file.
Then let's just use - or so to indicate the inferred name, with a "string"
overriding it?
Greetings,
Andres Freund
On Thu, Jul 06, 2023 at 06:19:43PM -0700, Andres Freund wrote: > On 2023-07-06 09:36:12 +0900, Michael Paquier wrote: >> So you mean renaming the existing events like WalSenderWaitForWAL to >> WalSenderWaitForWal? > > Yes. > >> The impact on existing monitoring queries is not zero because any changes >> would be silent, and that's the part that worried me the most even if it can >> remove one column in the txt file. > > Then let's just use - or so to indicate the inferred name, with a "string" > overriding it? Hmm. If we go down this road I would make the choice of simplicity and remove entirely a column, then, generating the snakecase from the camelcase or vice-versa (say like a $string =~ s/([a-z]+)/$1_/g;), even if it means having slightly incompatible strings showing to the users. And I'd rather minimize the number of exceptions we need to handle in this automation (aka no exception rules for some keywords like "SSL" or "WAL", etc.). -- Michael
Attachment
On Fri, Jul 07, 2023 at 01:49:24PM +0900, Michael Paquier wrote: > Hmm. If we go down this road I would make the choice of simplicity > and remove entirely a column, then, generating the snakecase from the > camelcase or vice-versa (say like a $string =~ s/([a-z]+)/$1_/g;), > even if it means having slightly incompatible strings showing to the > users. And I'd rather minimize the number of exceptions we need to > handle in this automation (aka no exception rules for some keywords > like "SSL" or "WAL", etc.). After pondering more about that, the attached patch set does exactly that. Patch 0001 includes an update of the wait event names so as these are more consistent with the enum elements generated. With this change, users can apply lower() or upper() across monitoring queries and still get the same results as before. An exception was the message queue events, which the enums used "MQ" but the event names used "MessageQueue", but this concerns only four lines of code in the backend. The newly-generated enum elements match with the existing ones, except for MQ. Patch 0002 introduces a set of simplifications for the format of wait_event_names.txt: - Removal of the first column for the enums. - Removal of the quotes for the event name. We have a single keyword for these, so that's kind of annoying to cope with that for new entries. - Build of the enum elements using the event names, by applying a rebuild as simple as that: + $waiteventenumname =~ s/([a-z])([A-Z])/$1_$2/g; + $waiteventenumname = uc($waiteventenumname); Thoughts? -- Michael
Attachment
Hi, On 7/9/23 6:32 AM, Michael Paquier wrote: > On Fri, Jul 07, 2023 at 01:49:24PM +0900, Michael Paquier wrote: >> Hmm. If we go down this road I would make the choice of simplicity >> and remove entirely a column, then, generating the snakecase from the >> camelcase or vice-versa (say like a $string =~ s/([a-z]+)/$1_/g;), >> even if it means having slightly incompatible strings showing to the >> users. And I'd rather minimize the number of exceptions we need to >> handle in this automation (aka no exception rules for some keywords >> like "SSL" or "WAL", etc.). > > After pondering more about that, the attached patch set does exactly > that. Thanks! > Patch 0001 includes an update of the wait event names so as > these are more consistent with the enum elements generated. With this > change, users can apply lower() or upper() across monitoring queries > and still get the same results as before. An exception was the > message queue events, which the enums used "MQ" but the event names > used "MessageQueue", but this concerns only four lines of code in the > backend. The newly-generated enum elements match with the existing > ones, except for MQ. > > Patch 0002 introduces a set of simplifications for the format of > wait_event_names.txt: > - Removal of the first column for the enums. > - Removal of the quotes for the event name. We have a single keyword > for these, so that's kind of annoying to cope with that for new > entries. > - Build of the enum elements using the event names, by applying a > rebuild as simple as that: > + $waiteventenumname =~ s/([a-z])([A-Z])/$1_$2/g; > + $waiteventenumname = uc($waiteventenumname); > > Thoughts? That's great and it does simplify the wait_event_names.txt format (and the impact on "MQ" does not seem like a big deal). I also noticed that you now provide the culprit line in case of parsing failure (thanks for that). # -# "C symbol in enums" "format in the system views" "description in the docs" +# "format in the system views" "description in the docs" Should we add a note here about the impact of the "format in the system views" on the auto generated enum? (aka how it is generated based on its format)? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Sun, Jul 09, 2023 at 09:15:34AM +0200, Drouvot, Bertrand wrote: > I also noticed that you now provide the culprit line in case of parsing > failure (thanks for that). Yes, that's mentioned in the commit message I quickly wrote in 0002. > # > -# "C symbol in enums" "format in the system views" "description in the docs" > +# "format in the system views" "description in the docs" > > Should we add a note here about the impact of the "format in the system views" on > the auto generated enum? (aka how it is generated based on its format)? There is one, but now that I look at it WAIT_EVENT repeated twice does not look great, so this could use "FooBarName" or equivalent: + # Generate the element name for the enums based on the + # description. Camelcase strings like "WaitEventName" + # are converted to WAIT_EVENT_WAIT_EVENT_NAME. -- Michael
Attachment
Hi, On 7/9/23 9:36 AM, Michael Paquier wrote: > On Sun, Jul 09, 2023 at 09:15:34AM +0200, Drouvot, Bertrand wrote: >> I also noticed that you now provide the culprit line in case of parsing >> failure (thanks for that). > > Yes, that's mentioned in the commit message I quickly wrote in 0002. > >> # >> -# "C symbol in enums" "format in the system views" "description in the docs" >> +# "format in the system views" "description in the docs" >> >> Should we add a note here about the impact of the "format in the system views" on >> the auto generated enum? (aka how it is generated based on its format)? > > There is one, Yeah there is one in generate-wait_event_types.pl. I was wondering to add one in wait_event_names.txt too (as this is the place where no wait events would be added if any). > but now that I look at it WAIT_EVENT repeated twice does > not look great, so this could use "FooBarName" or equivalent: +1 for "FooBarName" Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Mon, Jul 10, 2023 at 07:05:30AM +0200, Drouvot, Bertrand wrote: > Yeah there is one in generate-wait_event_types.pl. I was wondering > to add one in wait_event_names.txt too (as this is the place where > no wait events would be added if any). Hmm. Something like that could be done, for instance: # src/backend/utils/activity/wait_event_types.h -# typedef enum definitions for wait events. +# typedef enum definitions for wait events, generated from the first +# field. -- Michael
Attachment
On 7/10/23 7:20 AM, Michael Paquier wrote: > On Mon, Jul 10, 2023 at 07:05:30AM +0200, Drouvot, Bertrand wrote: >> Yeah there is one in generate-wait_event_types.pl. I was wondering >> to add one in wait_event_names.txt too (as this is the place where >> no wait events would be added if any). > > Hmm. Something like that could be done, for instance: > > # src/backend/utils/activity/wait_event_types.h > -# typedef enum definitions for wait events. > +# typedef enum definitions for wait events, generated from the first > +# field. Yeah, it looks a good place for it. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 2023-Jul-09, Michael Paquier wrote: > Patch 0002 introduces a set of simplifications for the format of > wait_event_names.txt: > - Removal of the first column for the enums. I don't like this bit, because it means the .txt file is now ungreppable as source of the enum name. Things become mysterious and people have to track down the event name by reading the the Perl generating script. It's annoying. I'd rather have the extra column, even if it means a little duplicity. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)
On Mon, Jul 10, 2023 at 09:11:36AM +0200, Alvaro Herrera wrote: > I don't like this bit, because it means the .txt file is now ungreppable > as source of the enum name. Things become mysterious and people have to > track down the event name by reading the the Perl generating script. > It's annoying. I'd rather have the extra column, even if it means a > little duplicity. Hmm. I can see your point that we'd lose the direct relationship between the enum and string when running a single `git grep` from the tree, still attempting to do that does not actually lead to much information gained? Personally, I usually grep for code when looking for consistent information across various paths in the tree. Wait events are very different: each enum is used in a single place in the tree making their grep search the equivalent of looking at wait_event_names.txt anyway? The quotes in the second columns can be removed even with your argument in place. That improves a bit the format. -- Michael
Attachment
On 7/11/23 12:52 AM, Michael Paquier wrote:
> On Mon, Jul 10, 2023 at 09:11:36AM +0200, Alvaro Herrera wrote:
>> I don't like this bit, because it means the .txt file is now ungreppable
>> as source of the enum name. Things become mysterious and people have to
>> track down the event name by reading the the Perl generating script.
>> It's annoying. I'd rather have the extra column, even if it means a
>> little duplicity.
>
> Hmm. I can see your point that we'd lose the direct relationship
> between the enum and string when running a single `git grep` from the
> tree, still attempting to do that does not actually lead to much
> information gained? Personally, I usually grep for code when looking
> for consistent information across various paths in the tree. Wait
> events are very different: each enum is used in a single place in the
> tree making their grep search the equivalent of looking at
> wait_event_names.txt anyway?
>
Before commit fa88928470 one could find the relationship between the enum and the name
in wait_event.c (a simple git grep would provide it).
With commit fa88928470 in place, one could find the relationship between the enum and the name
in wait_event_names.txt (a simple git grep would provide it).
With the proposal we are discussing here, once the build is done and so the pgstat_wait_event.c
file is generated then we have the same "grep" capability than pre commit fa88928470 (except that
"git grep" can't be used and one would need things like
find . -name "*.c" -exec grep -il "WAIT_EVENT_CHECKPOINTER_MAIN" {} \;)
I agree that it is less "obvious" than pre fa88928470 but still doable though.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Mon, Jul 10, 2023 at 07:52:23AM +0200, Drouvot, Bertrand wrote: > On 7/10/23 7:20 AM, Michael Paquier wrote: >> Hmm. Something like that could be done, for instance: >> >> # src/backend/utils/activity/wait_event_types.h >> -# typedef enum definitions for wait events. >> +# typedef enum definitions for wait events, generated from the first >> +# field. > > Yeah, it looks a good place for it. I am not sure where we are on that based on the objection from Alvaro to not remove the first column in wait_event_names.txt about greppability. Anyway, I am not seeing any objections behind my suggestion to simplify the second column and remove the quotes from the event names, either. Besides, the suggestion of Andres to improve the error message on parsing and show the line information is something useful in itself. Hence, attached is a rebased patch set that separates the work into more patches: - 0001 removes the quotes from the second column, improving the readability of the .txt file. - 0002 fixes the report from Andres to improve the error message on parsing. - 0003 is the rename of the wait events, in preparation for... - 0004 that removes entirely the first column (enum element names) from wait_event_names.txt. I would like to apply 0001 and 0002 to improve the format if there are no objections. 0003 and 0004 are still here for discussion, as it does not seem like a consensus has been reached for that yet. Getting more opinions would be a good next step for the last two patches, I assume. So, any comments? -- Michael
Attachment
On Thu, Jul 13, 2023 at 10:26:54AM +0900, Michael Paquier wrote: > I would like to apply 0001 and 0002 to improve the format if there are > no objections. 0003 and 0004 are still here for discussion, as it > does not seem like a consensus has been reached for that yet. Getting > more opinions would be a good next step for the last two patches, I > assume. I have looked again at 0001 and 0002 and applied them to get them out of the way. 0003 and 0004 are rebased and attached. I'll add them to the CF for later consideration. More opinions are welcome. -- Michael
Attachment
Hi, On 2023-07-14 13:49:22 +0900, Michael Paquier wrote: > I have looked again at 0001 and 0002 and applied them to get them out > of the way. 0003 and 0004 are rebased and attached. I'll add them to > the CF for later consideration. More opinions are welcome. > From b6390183bdcc054df82279bb0b2991730f85a0a3 Mon Sep 17 00:00:00 2001 > From: Michael Paquier <michael@paquier.xyz> > Date: Thu, 13 Jul 2023 10:14:47 +0900 > Subject: [PATCH v3 4/4] Remove column for enum elements in > wait_event_names.txt > > This file is now made of two columns, removing the column listing the > enum elements for each wait event class: > - Camelcase event name used in pg_stat_activity. There are now > unquoted. > - Description of the documentation. > The enum elements are generated from what is now the first column. I think the search issue is valid, so I do think going the other way is preferrable. I.e. use just the enum value in the .txt and generate the camel case name from that. That allows you to search the define used in code and find a hit in the file. I personally would still leave off the WAIT_EVENT prefix in the .txt, I think most of us can remember to chop that off. I don't think we need to be particularly consistent with wait events across major versions. They're necessarily tied to how the code works, and we've yanked that around plenty. Greetings, Andres Freund
On Sun, Jul 16, 2023 at 12:21:20PM -0700, Andres Freund wrote: > I think the search issue is valid, so I do think going the other way is > preferrable. I.e. use just the enum value in the .txt and generate the camel > case name from that. That allows you to search the define used in code and > find a hit in the file. > > I personally would still leave off the WAIT_EVENT prefix in the .txt, I think > most of us can remember to chop that off. So you mean to switch a line that now looks like that: WAIT_EVENT_FOO_BAR FooBar "Waiting on Foo Bar." To that: FOO_BAR "Waiting on Foo Bar." Or even that: WAIT_EVENT_FOO_BAR "Waiting on Foo Bar." Sure, it is an improvement for any wait events that use WAIT_EVENT_ when searching them, but this adds more magic into the LWLock and Lock areas if the same conversion is applied there. Or am I right to assume that you'd mean to *not* do any of that for these two classes? These can be treated as exceptions in the script when generating the wait event names from the enum elements, of course. > I don't think we need to be particularly consistent with wait events across > major versions. They're necessarily tied to how the code works, and we've > yanked that around plenty. IMO, it depends on the code path involved. For example, I know of some code that relies on SyncRep to track backends waiting on a sync reply, and that's one sensible to keep compatible. I'd be sad if something like that breaks suddenly after a major release. -- Michael
Attachment
On Mon, Jul 17, 2023 at 10:16:02AM +0900, Michael Paquier wrote: > So you mean to switch a line that now looks like that: > WAIT_EVENT_FOO_BAR FooBar "Waiting on Foo Bar." > To that: > FOO_BAR "Waiting on Foo Bar." > Or even that: > WAIT_EVENT_FOO_BAR "Waiting on Foo Bar." > > Sure, it is an improvement for any wait events that use WAIT_EVENT_ > when searching them, but this adds more magic into the LWLock and Lock > areas if the same conversion is applied there. Or am I right to > assume that you'd mean to *not* do any of that for these two classes? > These can be treated as exceptions in the script when generating the > wait event names from the enum elements, of course. I have looked again at that, and switching wait_event_names.txt to use two columns made of the typedef definitions and the docs like is not a problem: FOO_BAR "Waiting on Foo Bar." WAIT_EVENT_ is appended to the typedef definitions in the script. The wait event names like "FooBar" are generated from the enums by splitting using their underscores and doing some lc(). Lock and LWLock don't need to change. This way, it is easy to grep the wait events from the source code and match them with wait_event_names.txt. Thoughts or comments? -- Michael
Attachment
Hi, On 8/28/23 10:04 AM, Michael Paquier wrote: > On Mon, Jul 17, 2023 at 10:16:02AM +0900, Michael Paquier wrote: >> So you mean to switch a line that now looks like that: >> WAIT_EVENT_FOO_BAR FooBar "Waiting on Foo Bar." >> To that: >> FOO_BAR "Waiting on Foo Bar." >> Or even that: >> WAIT_EVENT_FOO_BAR "Waiting on Foo Bar." >> >> Sure, it is an improvement for any wait events that use WAIT_EVENT_ >> when searching them, but this adds more magic into the LWLock and Lock >> areas if the same conversion is applied there. Or am I right to >> assume that you'd mean to *not* do any of that for these two classes? >> These can be treated as exceptions in the script when generating the >> wait event names from the enum elements, of course. > > I have looked again at that, and switching wait_event_names.txt to use > two columns made of the typedef definitions and the docs like is not a > problem: > FOO_BAR "Waiting on Foo Bar." > > WAIT_EVENT_ is appended to the typedef definitions in the script. The > wait event names like "FooBar" are generated from the enums by > splitting using their underscores and doing some lc(). Lock and > LWLock don't need to change. This way, it is easy to grep the wait > events from the source code and match them with wait_event_names.txt. > > Thoughts or comments? Agree that done that way one could easily grep the events from the source code and match them with wait_event_names.txt. Then I don't think the "search" issue in the code is still a concern with the current proposal. FWIW, I'm getting: $ git am v3-000* Applying: Rename wait events with more consistent camelcase style Applying: Remove column for wait event names in wait_event_names.txt error: patch failed: src/backend/utils/activity/wait_event_names.txt:261 error: src/backend/utils/activity/wait_event_names.txt: patch does not apply Patch failed at 0002 Remove column for wait event names in wait_event_names.txt Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Tue, Aug 29, 2023 at 08:17:10AM +0200, Drouvot, Bertrand wrote: > Agree that done that way one could easily grep the events from the > source code and match them with wait_event_names.txt. Then I don't > think the "search" issue in the code is still a concern with the > current proposal. It could still be able to append WAIT_EVENT_ to the first column of the file. I'd just rather keep it shorter. > FWIW, I'm getting: > > $ git am v3-000* > Applying: Rename wait events with more consistent camelcase style > Applying: Remove column for wait event names in wait_event_names.txt > error: patch failed: src/backend/utils/activity/wait_event_names.txt:261 > error: src/backend/utils/activity/wait_event_names.txt: patch does not apply > Patch failed at 0002 Remove column for wait event names in wait_event_names.txt That may be a bug in the matrix because of bb90022, as git am can be easily pissed. I am attaching a new patch series, but it does not seem to matter here. I have double-checked the docs generated, while on it, and I am not seeing anything missing, particularly for the LWLock and Lock parts.. -- Michael
Attachment
On 2023-Aug-29, Michael Paquier wrote: > On Tue, Aug 29, 2023 at 08:17:10AM +0200, Drouvot, Bertrand wrote: > > Agree that done that way one could easily grep the events from the > > source code and match them with wait_event_names.txt. Then I don't > > think the "search" issue in the code is still a concern with the > > current proposal. > > It could still be able to append WAIT_EVENT_ to the first column of > the file. I'd just rather keep it shorter. Yeah, I have a mild preference for keeping the prefix, but it's mild because I also imagine that if somebody doesn't see the full symbol name when grepping they will think to remove the prefix. So only -0.1. I think the DOCONLY stuff should be better documented; they make no sense without looking at the commit message for fa88928470b5. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On Tue, Aug 29, 2023 at 02:21:48PM +0200, Alvaro Herrera wrote: > Yeah, I have a mild preference for keeping the prefix, but it's mild > because I also imagine that if somebody doesn't see the full symbol name > when grepping they will think to remove the prefix. So only -0.1. So, are you fine with the patch as presented? Or are there other things you'd like to see changed in the format? > I think the DOCONLY stuff should be better documented; they make no > sense without looking at the commit message for fa88928470b5. Good point. However, with 0002 in place these are gone. -- Michael
Attachment
Hi,
On 8/29/23 8:41 AM, Michael Paquier wrote:
> On Tue, Aug 29, 2023 at 08:17:10AM +0200, Drouvot, Bertrand wrote:
> That may be a bug in the matrix because of bb90022, as git am can be
> easily pissed.
git am does not complain anymore.
+ # Generate the element name for the enums based on the
+ # description. The C symbols are prefixed with "WAIT_EVENT_".
Nit: 2 whitespaces before "The C"
# Build the descriptions. There are in camel-case.
# LWLock and Lock classes do not need any modifications.
Nit: 2 whitespaces before "There are in camel"
+ my $waiteventdescription = '';
+ if ( $waitclassname eq 'WaitEventLWLock'
Nit: Too many whitespace after the "if (" ?? (I guess pgperltidy would
fix it).
> I have double-checked the docs generated, while on it, and I am not
> seeing anything missing, particularly for the LWLock and Lock parts..
I did compare the output of select * from pg_wait_events order by 1,2 and
ignored the case (with and without the patch series).
Then, the only diff is:
< Client,WalSenderWaitWal,Waiting for WAL to be flushed in WAL sender process
---
> Client,WalSenderWaitForWAL,Waiting for WAL to be flushed in WAL sender process
That said, it looks good to me.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Mon, Sep 04, 2023 at 02:14:58PM +0200, Drouvot, Bertrand wrote:
> # Build the descriptions. There are in camel-case.
> # LWLock and Lock classes do not need any modifications.
>
> Nit: 2 whitespaces before "There are in camel"
The whitespaces are intentional, the typo in the first line is not.
> + my $waiteventdescription = '';
> + if ( $waitclassname eq 'WaitEventLWLock'
>
> Nit: Too many whitespace after the "if (" ?? (I guess pgperltidy would
> fix it).
Here, perltidy is indeed complaining, but it is adding a few
whitespaces.
> Then, the only diff is:
>
> < Client,WalSenderWaitWal,Waiting for WAL to be flushed in WAL sender process
> ---
> > Client,WalSenderWaitForWAL,Waiting for WAL to be flushed in WAL sender process
>
> That said, it looks good to me.
Ah, good catch. I did not think about cross-checking the data in the
new view before and after the patch set. This rename needs to happen
in 0001.
Please find v5 attached. How does that look?
--
Michael
Attachment
Hi, On 9/5/23 7:44 AM, Michael Paquier wrote: > On Mon, Sep 04, 2023 at 02:14:58PM +0200, Drouvot, Bertrand wrote: >> # Build the descriptions. There are in camel-case. >> # LWLock and Lock classes do not need any modifications. >> >> Nit: 2 whitespaces before "There are in camel" > > The whitespaces are intentional, Oh ok, out of curiosity, why are 2 whitespaces intentional? >> Then, the only diff is: >> >> < Client,WalSenderWaitWal,Waiting for WAL to be flushed in WAL sender process >> --- >>> Client,WalSenderWaitForWAL,Waiting for WAL to be flushed in WAL sender process >> >> That said, it looks good to me. > > Ah, good catch. I did not think about cross-checking the data in the > new view before and after the patch set. This rename needs to happen > in 0001. > > Please find v5 attached. How does that look? Thanks! That looks good. I just noticed that v5 did re-introduce the "issue" that was fixed in 00e49233a9. Also, v5 needs a rebase due to f691f5b80a. Attaching v6 taking care of the 2 points mentioned above. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, Sep 05, 2023 at 11:06:36AM +0200, Drouvot, Bertrand wrote: > Oh ok, out of curiosity, why are 2 whitespaces intentional? That depends on the individual who write the code, but I recall that this is some old-school style from the 70's and/or the 80's when typing machines were still something. I'm just used to this style after the end of a sentence in a comment. > That looks good. I just noticed that v5 did re-introduce the "issue" that > was fixed in 00e49233a9. > > Also, v5 needs a rebase due to f691f5b80a. > > Attaching v6 taking care of the 2 points mentioned above. Dammit, thanks. These successive rebases are a bit annoying.. The data produced is consistent, and the new contents can be grepped, so I think that I am just going to apply both patches and move on to other topics. -- Michael
Attachment
On 05/09/2023 13:50 CEST Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Sep 05, 2023 at 11:06:36AM +0200, Drouvot, Bertrand wrote: > > Oh ok, out of curiosity, why are 2 whitespaces intentional? > > That depends on the individual who write the code, but I recall that > this is some old-school style from the 70's and/or the 80's when > typing machines were still something. I'm just used to this style > after the end of a sentence in a comment. FYI: https://en.wikipedia.org/wiki/Sentence_spacing -- Erik
On Tue, Sep 05, 2023 at 11:06:36AM +0200, Drouvot, Bertrand wrote: > Also, v5 needs a rebase due to f691f5b80a. > > Attaching v6 taking care of the 2 points mentioned above. Thanks. This time I have correctly checked the consistency of the data produced across all these commits using pg_wait_events, and that's OK. So applied both. -- Michael
Attachment
On Wed, Sep 06, 2023 at 04:20:23AM +0200, Erik Wienhold wrote: > FYI: https://en.wikipedia.org/wiki/Sentence_spacing That was an interesting read. Thanks. -- Michael
Attachment
Hi, On 9/6/23 5:44 AM, Michael Paquier wrote: > On Wed, Sep 06, 2023 at 04:20:23AM +0200, Erik Wienhold wrote: >> FYI: https://en.wikipedia.org/wiki/Sentence_spacing > > That was an interesting read. Thanks. +1, thanks! Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Wed, Jul 05, 2023 at 10:57:19AM +0900, Michael Paquier wrote: > I have applied it. I like the new developer experience of adding a wait event. After release of v17, how should we approach back-patching an event, like was done in commits 8fa4a1a 1396b5c 78c0f85? Each of those commits put the new event at the end of its released-branch wait_event.h enum. In v17, generate-wait_event_types.pl sorts events to position them. Adding an event will renumber others, which can make an extension report the wrong event until recompiled. Extensions citus, pg_bulkload, and vector refer to static events. If a back-patch added WAIT_EVENT_MESSAGE_QUEUE_SOMETHING_NEW, an old-build pg_bulkload report of WAIT_EVENT_PARALLEL_CREATE_INDEX_SCAN would show up in pg_stat_activity as WAIT_EVENT_PARALLEL_BITMAP_SCAN. (WAIT_EVENT_EXTENSION is not part of a generated enum, fortunately.) Some options: 1. Don't back-patch wait events to v17+. Use the closest existing event. 2. Let wait_event_names.txt back-patches control the enum order. For example, a line could have an annotation that controls its position relative to the auto-sorted lines. For another example, the generator could stop sorting. 3. Accept the renumbering, because the consequence isn't that horrible. Option (3) is worse than (1), but I don't have a recommendation between (1) and (2). I tend to like (1), a concern being the ease of accidental violations. If we had the ABI compliance checking that https://postgr.es/m/flat/CAH2-Wzk7tvgLXzOZ8a22aF-gmO5gHojWTYRvAk5ZgOvTrcEQeg@mail.gmail.com explored, (1) would be plenty safe. Should anything change here, or not?
On Sun, Mar 17, 2024 at 11:31:14AM -0700, Noah Misch wrote: > I like the new developer experience of adding a wait event. After release of > v17, how should we approach back-patching an event, like was done in commits > 8fa4a1a 1396b5c 78c0f85? Each of those commits put the new event at the end > of its released-branch wait_event.h enum. In v17, > generate-wait_event_types.pl sorts events to position them. Indeed, that would be a bad idea. > Adding an event > will renumber others, which can make an extension report the wrong event until > recompiled. Extensions citus, pg_bulkload, and vector refer to static events. > If a back-patch added WAIT_EVENT_MESSAGE_QUEUE_SOMETHING_NEW, an old-build > pg_bulkload report of WAIT_EVENT_PARALLEL_CREATE_INDEX_SCAN would show up in > pg_stat_activity as WAIT_EVENT_PARALLEL_BITMAP_SCAN. (WAIT_EVENT_EXTENSION is > not part of a generated enum, fortunately.) Some options: > > 1. Don't back-patch wait events to v17+. Use the closest existing event. > 2. Let wait_event_names.txt back-patches control the enum order. For example, > a line could have an annotation that controls its position relative to the > auto-sorted lines. For another example, the generator could stop sorting. > 3. Accept the renumbering, because the consequence isn't that horrible. > > Option (3) is worse than (1), but I don't have a recommendation between (1) > and (2). I tend to like (1), a concern being the ease of accidental > violations. If we had the ABI compliance checking that > https://postgr.es/m/flat/CAH2-Wzk7tvgLXzOZ8a22aF-gmO5gHojWTYRvAk5ZgOvTrcEQeg@mail.gmail.com > explored, (1) would be plenty safe. Should anything change here, or not? (1) would be annoying, we have backpatched scaling problems in the past even if that does not happen often. And in some cases I can understand why one would want to add a new wait event to track that a patch does what is expected of it. (2) to stop the automated sorting would bring back the problems that this thread has spent time to solve: people tend to not add wait events correctly, so I would suspect issues on HEAD. I've seen that too many times on older branches. I see an option (4), similar to your (2) without the per-line annotation: add a new magic keyword like the existing "Section" that is used in the first lines of generate-wait_event_types.pl where we generate tab-separated lines with the section name as prefix of each line. So I can think of something like: Section: ClassName - WaitEventFoo FOO_1 "Waiting in foo1" FOO_2 "Waiting in foo2" Backpatch: BAR_1 "Waiting in bar1" BAR_2 "Waiting in bar2" Then force the ordering for the docs and keep the elements in the backpatch section at the end of the enums in the order in the txt. One thing that could make sense is to enforce that "Backpatch" is at the end of a section, meaning that we would need a second keyword like a "Section: EndBackpatch" or something like that. That's not strictly necessary IMO as the format of the txt is easy to follow. -- Michael
Attachment
On Mon, Mar 18, 2024 at 08:02:24AM +0900, Michael Paquier wrote: > > 1. Don't back-patch wait events to v17+. Use the closest existing event. > > 2. Let wait_event_names.txt back-patches control the enum order. For example, > > a line could have an annotation that controls its position relative to the > > auto-sorted lines. For another example, the generator could stop sorting. > > 3. Accept the renumbering, because the consequence isn't that horrible. > I see an option (4), similar to your (2) without the per-line > annotation: add a new magic keyword like the existing "Section" that > is used in the first lines of generate-wait_event_types.pl where we > generate tab-separated lines with the section name as prefix of each > line. So I can think of something like: > Section: ClassName - WaitEventFoo > FOO_1 "Waiting in foo1" > FOO_2 "Waiting in foo2" > Backpatch: > BAR_1 "Waiting in bar1" > BAR_2 "Waiting in bar2" > > Then force the ordering for the docs and keep the elements in the > backpatch section at the end of the enums in the order in the txt. > One thing that could make sense is to enforce that "Backpatch" is at > the end of a section, meaning that we would need a second keyword like > a "Section: EndBackpatch" or something like that. That's not strictly > necessary IMO as the format of the txt is easy to follow. Works for me, with or without the trailing keyword line.
Hi, On Mon, Mar 18, 2024 at 08:02:24AM +0900, Michael Paquier wrote: > On Sun, Mar 17, 2024 at 11:31:14AM -0700, Noah Misch wrote: > > Adding an event > > will renumber others, which can make an extension report the wrong event until > > recompiled. Extensions citus, pg_bulkload, and vector refer to static events. > > If a back-patch added WAIT_EVENT_MESSAGE_QUEUE_SOMETHING_NEW, an old-build > > pg_bulkload report of WAIT_EVENT_PARALLEL_CREATE_INDEX_SCAN would show up in > > pg_stat_activity as WAIT_EVENT_PARALLEL_BITMAP_SCAN. (WAIT_EVENT_EXTENSION is > > not part of a generated enum, fortunately.) Nice catch, thanks! > I see an option (4), similar to your (2) without the per-line > annotation: add a new magic keyword like the existing "Section" that > is used in the first lines of generate-wait_event_types.pl where we > generate tab-separated lines with the section name as prefix of each > line. So I can think of something like: > Section: ClassName - WaitEventFoo > FOO_1 "Waiting in foo1" > FOO_2 "Waiting in foo2" > Backpatch: > BAR_1 "Waiting in bar1" > BAR_2 "Waiting in bar2" > > Then force the ordering for the docs and keep the elements in the > backpatch section at the end of the enums in the order in the txt. Yeah I think that's a good idea. > One thing that could make sense is to enforce that "Backpatch" is at > the end of a section, meaning that we would need a second keyword like > a "Section: EndBackpatch" or something like that. That's not strictly > necessary IMO as the format of the txt is easy to follow. I gave it a try in the POC patch attached. I did not use a "EndBackpatch" section to keep the perl script as simple a possible though (but documented the expectation instead). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On 2023-Aug-28, Michael Paquier wrote: > I have looked again at that, and switching wait_event_names.txt to use > two columns made of the typedef definitions and the docs like is not a > problem: > FOO_BAR "Waiting on Foo Bar." > > WAIT_EVENT_ is appended to the typedef definitions in the script. The > wait event names like "FooBar" are generated from the enums by > splitting using their underscores and doing some lc(). Lock and > LWLock don't need to change. This way, it is easy to grep the wait > events from the source code and match them with wait_event_names.txt. FTR I had a rather unpleasant time last week upon finding a wait event named PgSleep. If you grep for that, there are no matches at all; and I spent ten minutes (for real) trying to figure out where that was coming from, until I remembered this thread. Now you have to guess that not only random lowercasing is happening, but also underscore removal. This is not a good developer experience and I think we should rethink this choice. It would be infinitely more usable, and not one bit more difficult, to make these lines be WAIT_EVENT_FOO_BAR FooBar "Waiting on Foo Bar." then there is no guessing involved. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "The important things in the world are problems with society that we don't understand at all. The machines will become more complicated but they won't be more complicated than the societies that run them." (Freeman Dyson)
On Mon, Mar 18, 2024 at 09:04:44AM +0000, Bertrand Drouvot wrote: > --- a/src/backend/utils/activity/wait_event_names.txt > +++ b/src/backend/utils/activity/wait_event_names.txt > @@ -24,7 +24,12 @@ > # SGML tables of wait events for inclusion in the documentation. > # > # When adding a new wait event, make sure it is placed in the appropriate > -# ClassName section. > +# ClassName section. If the wait event is backpatched to a version < 17 then > +# put it under a "Backpatch" delimiter at the end of the related ClassName > +# section. Back-patch from v17 to pre-v17 won't use this, because v16 has hand-maintained enums. It's back-patch v18->v17 or v22->v17 where this will come up. > +# Ensure that the wait events under the "Backpatch" delimiter are placed in the > +# same order as in the pre 17 wait_event_types.h (see VERSION_FILE_SYNC as an > +# example). I expect the normal practice will be to put the entry in its natural position in git master, then put it in the backpatch section for any other branch. In other words, the backpatch regions are always empty in git master, and the non-backpatch regions change in master only.
Hi, On Mon, Mar 18, 2024 at 08:49:34AM -0700, Noah Misch wrote: > On Mon, Mar 18, 2024 at 09:04:44AM +0000, Bertrand Drouvot wrote: > > --- a/src/backend/utils/activity/wait_event_names.txt > > +++ b/src/backend/utils/activity/wait_event_names.txt > > @@ -24,7 +24,12 @@ > > # SGML tables of wait events for inclusion in the documentation. > > # > > # When adding a new wait event, make sure it is placed in the appropriate > > -# ClassName section. > > +# ClassName section. If the wait event is backpatched to a version < 17 then > > +# put it under a "Backpatch" delimiter at the end of the related ClassName > > +# section. > > Back-patch from v17 to pre-v17 won't use this, because v16 has hand-maintained > enums. It's back-patch v18->v17 or v22->v17 where this will come up. Thanks for looking at it! Oh right, the comment is wrong, re-worded in v2 attached. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Mon, Mar 18, 2024 at 08:49:34AM -0700, Noah Misch wrote: > On Mon, Mar 18, 2024 at 09:04:44AM +0000, Bertrand Drouvot wrote: >> +# Ensure that the wait events under the "Backpatch" delimiter are placed in the >> +# same order as in the pre 17 wait_event_types.h (see VERSION_FILE_SYNC as an >> +# example). > > I expect the normal practice will be to put the entry in its natural position > in git master, then put it in the backpatch section for any other branch. In > other words, the backpatch regions are always empty in git master, and the > non-backpatch regions change in master only. Yes, I'd expect the same experience. And it is very important to document that properly in the txt file. I don't see a need to specify any version numbers as well, that's less burden when bumping the major version number on the master branch every year. -- Michael
Attachment
On Mon, Mar 18, 2024 at 10:24:00AM +0100, Alvaro Herrera wrote: > FTR I had a rather unpleasant time last week upon finding a wait event > named PgSleep. If you grep for that, there are no matches at all; and I > spent ten minutes (for real) trying to figure out where that was coming > from, until I remembered this thread. > > Now you have to guess that not only random lowercasing is happening, but > also underscore removal. This is not a good developer experience and I > think we should rethink this choice. It would be infinitely more > usable, and not one bit more difficult, to make these lines be > > WAIT_EVENT_FOO_BAR FooBar "Waiting on Foo Bar." > > then there is no guessing involved. This has already gone through a couple of adjustments in 59cbf60c0f2b and 183a60a628fe. The latter has led to the elimination of one column in the txt file, as a reply to the same kind of comments about the format of this file: https://www.postgresql.org/message-id/20230705215939.ulnfbr4zavb2x7ri%40awork3.anarazel.de FWIW, I still like better what we have currently, where it is possible to grep the enum values in the source code. -- Michael
Attachment
On Mon, Mar 18, 2024 at 05:57:02PM +0000, Bertrand Drouvot wrote: > Thanks for looking at it! > Oh right, the comment is wrong, re-worded in v2 attached. I've added a couple of fake events in my txt file, and this results in an ordering of the wait events in the docs while the backpatched wait events are added at the end of the enums, based on their order in the txt file. # When adding a new wait event, make sure it is placed in the appropriate -# ClassName section. +# ClassName section. If the wait event is backpatched from master to a version +# >= 17 then put it under a "Backpatch:" delimiter at the end of the related +# ClassName section (on the non master branches) or at its natural position on +# the master branch. +# Ensure that the backpatch regions are always empty on the master branch. I'd recommend to not mention a version number at all, as this would need a manual refresh each time a new stable branch is forked. Your solution is simpler than what I finished in mind when looking at the code yesterday, with the addition of a second array that's pushed to be at the end of the "sorted" lines ordered by the second column. That does the job. (Note that I'll go silent for some time; I'll handle this thread when I get back as this is not urgent.) -- Michael
Attachment
Hi, On Tue, Mar 19, 2024 at 09:59:35AM +0900, Michael Paquier wrote: > On Mon, Mar 18, 2024 at 05:57:02PM +0000, Bertrand Drouvot wrote: > > Thanks for looking at it! > > Oh right, the comment is wrong, re-worded in v2 attached. > > I've added a couple of fake events in my txt file, and this results in > an ordering of the wait events in the docs while the backpatched wait > events are added at the end of the enums, based on their order in the > txt file. Thanks for testing! > # When adding a new wait event, make sure it is placed in the appropriate > -# ClassName section. > +# ClassName section. If the wait event is backpatched from master to a version > +# >= 17 then put it under a "Backpatch:" delimiter at the end of the related > +# ClassName section (on the non master branches) or at its natural position on > +# the master branch. > +# Ensure that the backpatch regions are always empty on the master branch. > > I'd recommend to not mention a version number at all, as this would > need a manual refresh each time a new stable branch is forked. 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"). > Your solution is simpler than what I finished in mind when looking at > the code yesterday, with the addition of a second array that's pushed > to be at the end of the "sorted" lines ordered by the second column. > That does the job. Yeah. > (Note that I'll go silent for some time; I'll handle this thread when > I get back as this is not urgent.) Right and enjoy! Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
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. The position of the Backpatch regions were a bit incorrect (extra one in LWLock, and the one in Lock was not needed). 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. 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. 0001 is the patch with my tweaks. 0002 includes some dummy test data I've used to validate the whole. -- Michael
Attachment
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
Attachment
On Thu, Apr 04, 2024 at 09:28:36AM +0000, Bertrand Drouvot wrote: > On Thu, Apr 04, 2024 at 03:50:21PM +0900, Michael Paquier wrote: >> 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. Okay. > 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. I'm OK with that. If somebody comes up wiht a better name than that, this could always be changed again. > +# 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. I'd add a "as -this section- has its own C code", for clarity. This just looked a bit strange here. > It looks like WAL_SENDER_WRITE_ZZZ was also added in it (I guess for testing > purpose, so I removed it in v4). That's a good brain fade. Thanks. -- Michael
Attachment
Hi, On Thu, Apr 04, 2024 at 07:14:47PM +0900, Michael Paquier wrote: > On Thu, Apr 04, 2024 at 09:28:36AM +0000, Bertrand Drouvot wrote: > > +# 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. > > I'd add a "as -this section- has its own C code", for clarity. This > just looked a bit strange here. Sounds good, done that way in v5 attached. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Attachment
On Thu, Apr 04, 2024 at 11:56:36AM +0000, Bertrand Drouvot wrote: > On Thu, Apr 04, 2024 at 07:14:47PM +0900, Michael Paquier wrote: >> I'd add a "as -this section- has its own C code", for clarity. This >> just looked a bit strange here. > > Sounds good, done that way in v5 attached. If there's a better suggestion than "ABI_compatibility" as keyword for this part of the file, this could always be changed later. For now, I've applied what you have here after tweaking a bit more the comments. -- Michael