Autogenerate some wait events code and documentation

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

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

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

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

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

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

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

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

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

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

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

[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

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

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]/messages/by-id/c6f35117-4b20-4c78-1df5-d3056010dcf5@gmail.com.

[1]: /messages/by-id/c6f35117-4b20-4c78-1df5-d3056010dcf5@gmail.com

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com