Re: Adding wait events statistics

On Mon, Jul 07, 2025 at 03:07:12PM +0000, Bertrand Drouvot wrote:
> On Mon, Jul 07, 2025 at 02:56:29PM +0900, Michael Paquier wrote:
>> On Mon, Jun 30, 2025 at 01:36:12PM +0000, Bertrand Drouvot wrote:
>
> Yeah, I think that this is needed for the custom wait events. But do we need
> to handle the custom wait events case? As, I think, the extensions have all they
> need at their disposal to count/increment their own counters when their custom
> wait events is triggered.

It's true that such extensions could create their own stats kind and
push their counters, but that's making the life of extension owners
more complicated, isn't it? We also have a few code paths where one
can push a custom wait event, like WaitLatch.

> I think the question is: if the extension owner does not increment it, do we want
> to do it in core on their behalf? I'm not sure as it's still up to them to make
> use of custom wait events: so some of them will, some not, making the "in core"
> counters not consistent depending of the extension. That could result in end
> users asking why they see counters for some and not for some.

My take would be yes. That feels more natural than ignoring things.

> I also have the feeling that adding this extra complexity for (I think) a really
> small number of cases may impact the "in core" wait events stats negatively (from
> a complexity and maybe performance points of view).

> I was thinking to just start with "in core" wait events and see if there is a
> need/ask to expand it to custom wait events. Thoughts?

This statement is perhaps true. Now we do use custom wait events even
in contrib/ modules, so the move feels natural to me.

> I'm not sure that would be simpler (or maybe that would simplify the perl
> script a bit) but I think that might complicate the C code a bit. I mean, given
> a wait_event_info we'd need to know in which array to increment the related wait
> event counter. So, (at least) some switch on the wait class would be needed (and
> that would probably need to be generated by the perl script, so I'm not sure
> that would simplify the perl script after all).

Noted.

> Also in pg_stat_get_wait_event(), we'd need to iterate on all the arrays so
> we'd need to know and identify them.

Yes, you may need some level of meta-data generated for the wait
classes generated when the perl script generating this data is run.
It would be nice to the footprint of the code generated minimal if we
can. It's one of these things where I would try both approaches, then
look at the diffs to conclude, but that's only my own time-consuming
way to approach such problems. It does not need to apply to others.

>>> That said it might be better to use all the 64 bits (means not have the half full
>>> of zeroes) for the hash key (better hashing distribution?): we could imagine
>>> using something like:
>>>
>>> ((uint64) wait_event_info) | (((uint64) wait_event_info) << 32)
>>>
>>> for the hash key.
>>
>> Using uint32 for the hash key is fine. At least it's not an issue for
>> the pgstats machinery.
>
> Yeah, my doubt was more about the hashing distribution if all the keys have
> the exact same bits (the upper 32 bits) sets to zero (which is what using a
> uint32 key would produce).

Hmm. Not sure about that, but that would not be difficult to check.
It is true that it would not be a good thing if all the stats get
pushed through the same partition in the pgstat dshash table, but I'm
pretty sure that we don't need to worry much about that, like for
neighboring OIDs.
--
Michael