Обсуждение: Re: Add AioUringCompletion in wait_event_names.txt and a safeguard in generate-wait_event_types.pl

On Mon, May 26, 2025 at 07:49:34AM +0000, Bertrand Drouvot wrote:
> c325a7633fcb forgot to add AioUringCompletion in wait_event_names.txt: please
> find attached v1-0001 fixing it (please double check the wait event description
> as I'm not that familiar with io_uring).

+AioUringCompletion "Waiting for another process to complete IO via io_uring."

"completion_lock" is described with similar words around the top of
method_io_uring.c.  In more exact words, we are waiting for a backend
to process IO completions with io_method=io_uring.  So perhaps:
"Waiting for another process to process IO completions with io_uring"

Adding Andres in CC for input, as I suspect that he'll think about a
better wording for the docs than what I could come up with.

> That makes me think that it is easy to miss adding a new LWLock in
> wait_event_names.txt and generate-wait_event_types.pl does not detect that.

Agreed, adding a check is a good idea seeing that the same mistake has
been repeated two times in the last couple of months by two different
committers.  This check is only in the docs, but the CI would detect
that.  This is not something strictly required for v18.  Let's tackle
that in v19~.

+   if (exists $hashwe{'WaitEventLWLock'})

So, the only reason why this is included in
generate-wait_event_types.pl is that we need the data from %hashwe,
reused in the new function you have added to perform the validation.

The implementation of your validation check is not consistent with the
existing generate-wait_event_types.pl, adding knowledge about lwlock.h
lwlocklist.h which are passed as extra arguments for the function
validate_lwlock_count().  Perhaps it would be better if split into its
own script, with the code in charge of building %hashwe (aka parsing
the .txt file given in input argument) moved to a .pm "Parsing"
module shared by both scripts, the one generating the data and the one
cross-checking the lwlock numbers?
--
Michael

On Tue, May 27, 2025 at 08:30:56AM +0900, Michael Paquier wrote:
> +AioUringCompletion "Waiting for another process to complete IO via io_uring."
>
> "completion_lock" is described with similar words around the top of
> method_io_uring.c.  In more exact words, we are waiting for a backend
> to process IO completions with io_method=io_uring.  So perhaps:
> "Waiting for another process to process IO completions with io_uring"

Hearing nothing, I've applied this one reusing the exact wording
suggested by Bertrand which is the most consistent choice with the
other wait events related to the same area of the code.
--
Michael

Hi,

On Tue, May 27, 2025 at 08:30:56AM +0900, Michael Paquier wrote:
> On Mon, May 26, 2025 at 07:49:34AM +0000, Bertrand Drouvot wrote:
> > That makes me think that it is easy to miss adding a new LWLock in
> > wait_event_names.txt and generate-wait_event_types.pl does not detect that.
> 
> Agreed, adding a check is a good idea

Thanks for sharing your thoughts! Yeah and avoid code/doc discrepancies was
also one of the reason for generate-wait_event_types.pl creation, so I think
that we must ensure that it does so.

> This check is only in the docs, but the CI would detect
> that.

Right.

> This is not something strictly required for v18.  Let's tackle
> that in v19~.

Yes.

> +   if (exists $hashwe{'WaitEventLWLock'})
> 
> So, the only reason why this is included in
> generate-wait_event_types.pl is that we need the data from %hashwe,
> reused in the new function you have added to perform the validation.
> 
> The implementation of your validation check is not consistent with the
> existing generate-wait_event_types.pl, adding knowledge about lwlock.h
> lwlocklist.h which are passed as extra arguments for the function
> validate_lwlock_count().

I'm not sure to get what you mean with "not consistent". I think that we need
lwlock.h and lwlocklist.h so that generate-wait_event_types.pl can do its
work efficiently i.e generate the code, doc and avoid code/doc discrepancies.

> Perhaps it would be better if split into its
> own script, with the code in charge of building %hashwe (aka parsing
> the .txt file given in input argument) moved to a .pm "Parsing"
> module shared by both scripts, the one generating the data and the one
> cross-checking the lwlock numbers?

That could probably work, but do we really need building a new .pl file? I think
that it would make more sense if the "new" .pl in charge of cross-checking the
lwlock numbers would do more checks (unrelated to generate-wait_event_types.pl).

Then, we could have something like the .pm, check_generated_code.pl (new pl) and
generate-wait_event_types.pl with check_generated_code.pl doing things that are
related or not to generate-wait_event_types.pl.

I feel that what is proposed here is really linked to generate-wait_event_types.pl
"only" (it's part of its duties) and that's probably why I'm having some difficulty
to see the pros of the split.

Regards,

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