Re: Autogenerate some wait events code and documentation

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