Обсуждение: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN

Hi hackers,

Please find attached a patch to $SUBJECT.

This is preliminary work to autogenerate some wait events
code and documentation done in [1].

The patch introduces 2 new "wait events" (WAIT_EVENT_EXTENSION
and WAIT_EVENT_BUFFER_PIN) and their associated functions
(pgstat_get_wait_extension() and pgstat_get_wait_bufferpin() resp.)

Please note that that would not break extensions outside contrib
that make use of the existing PG_WAIT_EXTENSION.

[1]: https://www.postgresql.org/message-id/flat/77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98@gmail.com

Looking forward to your feedback,

Regards,

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

Hi,

On 2023-05-15 10:07:04 +0200, Drouvot, Bertrand wrote:
> Please find attached a patch to $SUBJECT.
> 
> This is preliminary work to autogenerate some wait events
> code and documentation done in [1].
> 
> The patch introduces 2 new "wait events" (WAIT_EVENT_EXTENSION
> and WAIT_EVENT_BUFFER_PIN) and their associated functions
> (pgstat_get_wait_extension() and pgstat_get_wait_bufferpin() resp.)
> 
> Please note that that would not break extensions outside contrib
> that make use of the existing PG_WAIT_EXTENSION.
> 
> [1]: https://www.postgresql.org/message-id/flat/77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98@gmail.com
> 
> Looking forward to your feedback,

Without an explanation for why this change is needed for [1], it's hard to
give useful feedback...

Greetings,

Andres Freund

On Mon, May 15, 2023 at 11:29:56AM -0700, Andres Freund wrote:
> Without an explanation for why this change is needed for [1], it's hard to
> give useful feedback...

The point is to integrate the wait event classes for buffer pin and
extension into the txt file that automates the creation of the SGML
and C code associated to them.  Doing the refactoring of this patch
has two advantages:
- Being able to easily organize the tables for each wait event type
alphabetically, the same way as HEAD, for all wait event classes.
- Minimizing the number of exception rules needed in the perl script
that transforms the txt file into SGML and the C code to list all the
events associated in a class, where one function is used for each wait
event class.  Currently the wait event class extension does not use
that.

This impacts only the internal object names, not the wait event
strings or the class associated to them.  So this does not change the
contents of pg_stat_activity.
--
Michael

Hi,

On 2023-05-16 07:30:54 +0900, Michael Paquier wrote:
> On Mon, May 15, 2023 at 11:29:56AM -0700, Andres Freund wrote:
> > Without an explanation for why this change is needed for [1], it's hard to
> > give useful feedback...
> 
> The point is to integrate the wait event classes for buffer pin and
> extension into the txt file that automates the creation of the SGML
> and C code associated to them.

IMO the submission should include why automating requires these changes (yours
doesn't really either). I can probably figure it out if I stare a bit at the
code and read the other thread - but I shouldn't need to.

Greetings,

Andres Freund

On Mon, May 15, 2023 at 05:17:16PM -0700, Andres Freund wrote:
> IMO the submission should include why automating requires these changes (yours
> doesn't really either). I can probably figure it out if I stare a bit at the
> code and read the other thread - but I shouldn't need to.

Hm?  My previous message includes two reasons..  Anyway, I assume that
my previous message is also missing the explanation from the other
thread that this is to translate a .txt file shaped similarly to
errcodes.txt for the wait events (sections as wait event classes,
listing the elements) into automatically-generated SGML and C code :)

The extensions and buffer pin parts need a few internal tweaks to make
the other changes much easier to do, which is what the patch of this
thread is doing.
--
Michael

Hi,

On 2023-05-16 09:38:54 +0900, Michael Paquier wrote:
> On Mon, May 15, 2023 at 05:17:16PM -0700, Andres Freund wrote:
> > IMO the submission should include why automating requires these changes (yours
> > doesn't really either). I can probably figure it out if I stare a bit at the
> > code and read the other thread - but I shouldn't need to.
> 
> Hm?  My previous message includes two reasons..

It explained the motivation, but not why that requires the specific
changes. At least not in a way that I could quickly undestand.


> The extensions and buffer pin parts need a few internal tweaks to make
> the other changes much easier to do, which is what the patch of this
> thread is doing.

Why those tweaks are necessary is precisely what I am asking for.

Greetings,

Andres Freund

On Mon, May 15, 2023 at 06:01:02PM -0700, Andres Freund wrote:
> On 2023-05-16 09:38:54 +0900, Michael Paquier wrote:
>> On Mon, May 15, 2023 at 05:17:16PM -0700, Andres Freund wrote:
>>> IMO the submission should include why automating requires these changes (yours
>>> doesn't really either). I can probably figure it out if I stare a bit at the
>>> code and read the other thread - but I shouldn't need to.
>>
>> Hm?  My previous message includes two reasons..
>
> It explained the motivation, but not why that requires the specific
> changes. At least not in a way that I could quickly undestand.
>
>> The extensions and buffer pin parts need a few internal tweaks to make
>> the other changes much easier to do, which is what the patch of this
>> thread is doing.
>
> Why those tweaks are necessary is precisely what I am asking for.

Not sure how to answer to that without copy-pasting some code, and
that's what is written in the patch.  Anyway, I am used to the context
of what's wanted, so here you go with more details.  As a whole, the
patch reshapes the code to be more consistent for all the wait event
classes, so as it is possible to generate the code refactored by the
patch of this thread in a single way for all the classes.

The first change is related to the functions associated to each class,
used to fetch a specific wait event.  On HEAD, all the wait event classes
use a dedicated function (activity, IPC, etc.) like that:
        case PG_WAIT_BLAH:
    {
            WaitEventBlah w = (WaitEventBlah) wait_event_info;
            event_name = pgstat_get_wait_blah(w);
            break;
    }

There are two exceptions to that, the wait event classes for extension
and buffer pin, that just do that because these classes have only one
wait event currently:
        case PG_WAIT_EXTENSION:
           event_name = "Extension";
           break
        [...]
        case PG_WAIT_BUFFER_PIN:
            event_name = "BufferPin";
            break;
The first thing changed is to introduce two new functions for these
two classes, to work the same way as the other classes.  The script in
charge of generating the code from the wait event .txt file will just
build the internals of these functions.

The second change is to rework the enum structures for extension and
buffer pin, to be consistent with the other classes, so as all the
classes have structures shaped like that:
typedef enum
{
   WAIT_EVENT_1 = PG_WAIT_BLAH,
   [...]
   WAIT_EVENT_N
} WaitEventBlah;

Then the perl script generates the same structures for all the wait
event classes, with all the events associated to that.  There may be a
point in keeping extension and buffer pin out of this refactoring, but
reworking these like that moves in a single place all the wait event
definitions, making future additions easier.  Buffer pin actually
needed a small rename to stick with the naming rules of the other
classes.

These are the two things refactored in the patch, explaining the what.
The reason behind the why is to make the script in charge of
generating all these structures and functions consistent for all the
wait event classes, simply.  Treating all the wait event classes
together eases greatly the generation of the documentation, so that it
is possible to enforce an ordering of the tables of each class used to
list each wait event type attached to them.  Does this explanation
make sense?

Lock and LWLock are funkier because of the way they grab wait events
for the inner function, still these had better have their
documentation generated so as all the SGML tables created for all the
wait event tables are ordered alphabetically, in the same way as
HEAD.
--
Michael

Hi,

On 5/16/23 7:14 AM, Michael Paquier wrote:
> On Mon, May 15, 2023 at 06:01:02PM -0700, Andres Freund wrote:
>> On 2023-05-16 09:38:54 +0900, Michael Paquier wrote:
>>> On Mon, May 15, 2023 at 05:17:16PM -0700, Andres Freund wrote:

> These are the two things refactored in the patch, explaining the what.
> The reason behind the why is to make the script in charge of
> generating all these structures and functions consistent for all the
> wait event classes, simply.  Treating all the wait event classes
> together eases greatly the generation of the documentation, so that it
> is possible to enforce an ordering of the tables of each class used to
> list each wait event type attached to them.

Right, it does "fix" the ordering issue (for BufferPin and Extension)
that I've described in the patch introduction in [1]:

"
   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.
 
.
.
.
"

Thanks Michael for having provided this detailed explanation (my patch
introduction clearly was missing some context as Andres pointed out).

[1]: https://www.postgresql.org/message-id/77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98%40gmail.com

Regards,

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

On Mon, May 15, 2023 at 10:07:04AM +0200, Drouvot, Bertrand wrote:
> This is preliminary work to autogenerate some wait events
> code and documentation done in [1].
>
> The patch introduces 2 new "wait events" (WAIT_EVENT_EXTENSION
> and WAIT_EVENT_BUFFER_PIN) and their associated functions
> (pgstat_get_wait_extension() and pgstat_get_wait_bufferpin() resp.)
>
> Please note that that would not break extensions outside contrib
> that make use of the existing PG_WAIT_EXTENSION.

I have looked at this one, and I think that's OK for what you are
aiming at here (in addition to my previous message that I hope
provides enough context about the whys and the hows).
--
Michael

Hi,

On 5/16/23 8:16 AM, Michael Paquier wrote:
> On Mon, May 15, 2023 at 10:07:04AM +0200, Drouvot, Bertrand wrote:
>> This is preliminary work to autogenerate some wait events
>> code and documentation done in [1].
>>
>> The patch introduces 2 new "wait events" (WAIT_EVENT_EXTENSION
>> and WAIT_EVENT_BUFFER_PIN) and their associated functions
>> (pgstat_get_wait_extension() and pgstat_get_wait_bufferpin() resp.)
>>
>> Please note that that would not break extensions outside contrib
>> that make use of the existing PG_WAIT_EXTENSION.
> 
> I have looked at this one, and I think that's OK for what you are
> aiming at here (in addition to my previous message that I hope
> provides enough context about the whys and the hows).

Thanks!

Please find V2 attached, it adds WaitEventBufferPin and WaitEventExtension to
src/tools/pgindent/typedefs.list (that was done in [1]...).

[1]: https://www.postgresql.org/message-id/flat/77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98%40gmail.com

Regards,

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

On Tue, May 16, 2023 at 1:14 AM Michael Paquier <michael@paquier.xyz> wrote:
> These are the two things refactored in the patch, explaining the what.
> The reason behind the why is to make the script in charge of
> generating all these structures and functions consistent for all the
> wait event classes, simply.  Treating all the wait event classes
> together eases greatly the generation of the documentation, so that it
> is possible to enforce an ordering of the tables of each class used to
> list each wait event type attached to them.  Does this explanation
> make sense?

Not really. At least not to me. Changing the code in dblink to use
WAIT_EVENT_EXTENSION instead of PG_WAIT_EXTENSION doesn't help you
automatically generate documentation in any way.

It seems to me that your automatic generation code might need a
special case for wait event types that contain only a single wait
event. But that doesn't seem like a bad thing to have. Adding
pgstat_get_wait_extension adds runtime cost for no corresponding
benefit. Having a special case in the code to avoid that seems
worthwhile.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, May 17, 2023 at 09:22:19AM -0400, Robert Haas wrote:
> It seems to me that your automatic generation code might need a
> special case for wait event types that contain only a single wait
> event. But that doesn't seem like a bad thing to have. Adding
> pgstat_get_wait_extension adds runtime cost for no corresponding
> benefit. Having a special case in the code to avoid that seems
> worthwhile.

Okay.  We are going to need an approach similar to what's done for
src/backend/nodes where two things are generated in order to be able
to have some of the wait event classes be treated as exceptions in the
switch calling each function (pgstat_get_wait_event).  I'd assume:
- Create the code calling the functions automatically, say in a
wait_event_type.switch.c or something like that.  If a class has one
single element, generate the code from it.
- Create a second file with the functions and their internals, as the
patch does now (like wait_event_type.funcs.c?), discarding classes
with single elements.
- Skip the creation of the enum structures for single-element classes,
as well.

Still it looks like the renaming of BufferPin would need to remain
around to ease a bit the work of the script.  Bertrand, what do you
think?
--
Michael

On Thu, May 18, 2023 at 07:48:26AM +0900, Michael Paquier wrote:
> Okay.  We are going to need an approach similar to what's done for
> src/backend/nodes where two things are generated in order to be able
> to have some of the wait event classes be treated as exceptions in the
> switch calling each function (pgstat_get_wait_event).  I'd assume:
> - Create the code calling the functions automatically, say in a
> wait_event_type.switch.c or something like that.  If a class has one
> single element, generate the code from it.
> - Create a second file with the functions and their internals, as the
> patch does now (like wait_event_type.funcs.c?), discarding classes
> with single elements.
> - Skip the creation of the enum structures for single-element classes,
> as well.

On top of that, why don't we just apply some inlining to all the
pgstat_get_wait_*() functions?  If we do that, even the existing
functions could see a benefit on top of the ones associated to classes
with single elements.  Inlining may not be always applied depending on
what the compiler wants, of course..
--
Michael

On 2023-05-17 09:22:19 -0400, Robert Haas wrote:
> Adding pgstat_get_wait_extension adds runtime cost for no corresponding
> benefit. Having a special case in the code to avoid that seems worthwhile.

I don't think that should ever be used in a path where performance is
relevant?

On Wed, May 17, 2023 at 7:38 PM Andres Freund <andres@anarazel.de> wrote:
> On 2023-05-17 09:22:19 -0400, Robert Haas wrote:
> > Adding pgstat_get_wait_extension adds runtime cost for no corresponding
> > benefit. Having a special case in the code to avoid that seems worthwhile.
>
> I don't think that should ever be used in a path where performance is
> relevant?

I mean, I agree that it would probably be hard to measure any real
performance difference. But I'm not sure that's a good reason to add
cycles to a path where we don't really need to.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, May 18, 2023 at 12:28:20PM -0400, Robert Haas wrote:
> I mean, I agree that it would probably be hard to measure any real
> performance difference. But I'm not sure that's a good reason to add
> cycles to a path where we don't really need to.

Honestly, I am not sure that it's worth worrying about performance
here, or perhaps you know of some external stuff that could set the
extension class type in a code path hot enough that it would matter.
Anyway, why couldn't we make all these functions static inline
instead, then?
--
Michael

Hi,

On 5/19/23 12:36 AM, Michael Paquier wrote:
> On Thu, May 18, 2023 at 12:28:20PM -0400, Robert Haas wrote:
>> I mean, I agree that it would probably be hard to measure any real
>> performance difference. But I'm not sure that's a good reason to add
>> cycles to a path where we don't really need to.
> 
> Honestly, I am not sure that it's worth worrying about performance
> here,

Same feeling here and as those new functions will be used "only" from
pg_stat_get_activity() / pg_stat_get_backend_wait_event().

> or perhaps you know of some external stuff that could set the
> extension class type in a code path hot enough that it would matter.

And that would matter, only when making use of pg_stat_get_activity()
/ pg_stat_get_backend_wait_event() at the time the "extension is waiting"
on this wait event.

While at it, I think that making use of an enum might also be an open door
(need to think more about it) to allow extensions to set their own wait event.
Something like RequestNamedLWLockTranche()/GetNamedLWLockTranche() are doing.

Currently we have "only" the "extension" wait event which is not that useful when
there is multiples extensions installed in a database.

Regards,

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

Hi,

On 2023-05-19 16:48, Drouvot, Bertrand wrote:
> While at it, I think that making use of an enum might also be an open 
> door
> (need to think more about it) to allow extensions to set their own wait 
> event.
> Something like RequestNamedLWLockTranche()/GetNamedLWLockTranche() are 
> doing.
> 
> Currently we have "only" the "extension" wait event which is not that
> useful when
> there is multiples extensions installed in a database.

(Excuse me for cutting in, and this is not directly related to the 
thread.)
+1. I'm interested in the feature.

Recently, I encountered a case where it would be nice if
different wait events were output for each extension.

I tested a combination of two extensions, postgres_fdw and neon[1],
and they output the "Extension" wait event, but it wasn't immediately 
clear
which one was the bottleneck.

This is just a example and it probable be useful for other users. IMO, 
at least,
it's better to improve the specification that "Extension" wait event 
type has
only the "Extension" wait event.

[1] https://github.com/neondatabase/neon

Regards,

-- 
Masahiro Ikeda
NTT DATA CORPORATION

On Thu, Jun 08, 2023 at 10:57:55AM +0900, Masahiro Ikeda wrote:
> (Excuse me for cutting in, and this is not directly related to the thread.)
> +1. I'm interested in the feature.
>
> This is just a example and it probable be useful for other users. IMO, at
> least, it's better to improve the specification that "Extension"
> wait event type has only the "Extension" wait event.

I hope that nobody would counter-argue you here.  In my opinion, we
should just introduce an API that allows extensions to retrieve wait
event numbers that are allocated by the backend under
PG_WAIT_EXTENSION, in a fashion similar to GetNamedLWLockTranche().
Say something like:
int GetExtensionWaitEvent(const char *wait_event_name);

I don't quite see a design where extensions could rely on their own
numbers statically assigned by the extension maintainers, as this is
most likely going to cause conflicts.  And I would guess that a lot of
external code would want to get more data pushed to pg_stat_activity,
meaning a lot of conflicts, potentially.
--
Michael

Hi,

On 6/9/23 1:15 AM, Michael Paquier wrote:
> On Thu, Jun 08, 2023 at 10:57:55AM +0900, Masahiro Ikeda wrote:
>> (Excuse me for cutting in, and this is not directly related to the thread.)
>> +1. I'm interested in the feature.
>>
>> This is just a example and it probable be useful for other users. IMO, at
>> least, it's better to improve the specification that "Extension"
>> wait event type has only the "Extension" wait event.
> 
> I hope that nobody would counter-argue you here.  In my opinion, we
> should just introduce an API that allows extensions to retrieve wait
> event numbers that are allocated by the backend under
> PG_WAIT_EXTENSION, in a fashion similar to GetNamedLWLockTranche().
> Say something like:
> int GetExtensionWaitEvent(const char *wait_event_name);

+1, that's something I've in mind to work on once/if this patch is/get
committed.

Regards,

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

On Fri, Jun 09, 2023 at 06:26:07AM +0200, Drouvot, Bertrand wrote:
> +1, that's something I've in mind to work on once/if this patch is/get
> committed.

FWIW, I'm OK with the patch, without the need to worry about the
performance.  Even if that's the case, we could just mark all these as
inline and move on..
--
Michael

Hi,

On 2023-06-09 13:26, Drouvot, Bertrand wrote:
> Hi,
> 
> On 6/9/23 1:15 AM, Michael Paquier wrote:
>> On Thu, Jun 08, 2023 at 10:57:55AM +0900, Masahiro Ikeda wrote:
>>> (Excuse me for cutting in, and this is not directly related to the 
>>> thread.)
>>> +1. I'm interested in the feature.
>>> 
>>> This is just a example and it probable be useful for other users. 
>>> IMO, at
>>> least, it's better to improve the specification that "Extension"
>>> wait event type has only the "Extension" wait event.
>> 
>> I hope that nobody would counter-argue you here.  In my opinion, we
>> should just introduce an API that allows extensions to retrieve wait
>> event numbers that are allocated by the backend under
>> PG_WAIT_EXTENSION, in a fashion similar to GetNamedLWLockTranche().
>> Say something like:
>> int GetExtensionWaitEvent(const char *wait_event_name);
> 
> +1, that's something I've in mind to work on once/if this patch is/get
> committed.

Thanks for replying. If you are ok, I'll try to make a patch
to allow extensions to define custom wait events.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

Hi,

On 6/9/23 11:20 AM, Masahiro Ikeda wrote:
> Hi,
> 
> On 2023-06-09 13:26, Drouvot, Bertrand wrote:
>> Hi,
>>
>> On 6/9/23 1:15 AM, Michael Paquier wrote:
>>> On Thu, Jun 08, 2023 at 10:57:55AM +0900, Masahiro Ikeda wrote:
>>>> (Excuse me for cutting in, and this is not directly related to the thread.)
>>>> +1. I'm interested in the feature.
>>>>
>>>> This is just a example and it probable be useful for other users. IMO, at
>>>> least, it's better to improve the specification that "Extension"
>>>> wait event type has only the "Extension" wait event.
>>>
>>> I hope that nobody would counter-argue you here.  In my opinion, we
>>> should just introduce an API that allows extensions to retrieve wait
>>> event numbers that are allocated by the backend under
>>> PG_WAIT_EXTENSION, in a fashion similar to GetNamedLWLockTranche().
>>> Say something like:
>>> int GetExtensionWaitEvent(const char *wait_event_name);
>>
>> +1, that's something I've in mind to work on once/if this patch is/get
>> committed.
> 
> Thanks for replying. If you are ok, I'll try to make a patch
> to allow extensions to define custom wait events.

Great, thank you!

Regards,

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

On Fri, Jun 09, 2023 at 02:58:42PM +0900, Michael Paquier wrote:
> FWIW, I'm OK with the patch, without the need to worry about the
> performance.  Even if that's the case, we could just mark all these as
> inline and move on..

I am attempting to get all that done for the beginning of the
development cycle, so the first refactoring patch has been applied.
Now into the second, main one..
--
Michael