Re: Autogenerate some wait events code and documentation - Mailing list pgsql-hackers

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