Re: per backend I/O statistics

On Thu, Jan 16, 2025 at 11:28:43AM -0500, Andres Freund wrote:
> On 2025-01-15 18:27:22 +0900, Michael Paquier wrote:
>> My problem is that this is not only related to backend stats, but to
>> all variable-numbered stats kinds that require this behavior.
>
> The issue here imo is that recently IO stats were turned into a variable
> numbered stat, where it previously wasn't. It seems like a bad idea that now
> we regularly do a somewhat sizable memory allocation for backend stats. For
> table stats allocating the pending memory is somewhat a necessity - there
> could be a lot of different tables after all. And later transactions won't
> necessarily access the same set of tables, so we can't just keep the memory
> for pending around - but that's not true for IO.

Yep, for the reason that backend stats require that with one entry in
the pgstats dshash for each backend as these are sized depending on
max_connections & co using a proc number for the hash key.

>> Andres, do you have any thoughts about that? Not sure what would be
>> your view on this matter. MemoryContextAllowInCriticalSection() for
>> stats kinds that allow the case would be OK for me I guess, we should
>> not be talking about a lot of memory.
>
> My view is that for IO stats no memory allocation should be required - that
> used to be the case and should be the case again. And for something like
> injection points (not really convinced that it's a good thing to have stats
> for, but ...), allocation the shared memory stats object outside of the
> critical section and not having any pending stats seems to be the right
> approach.

Not sure to completely agree on that as this pushes the responsibility
of how to deal with the critical section handling to each stats kind,
but I won't fight over it for this case, either. It would not be the
first time we have specific logic to do allocations beforehand when
dealing with critical sections (38c579b08988 is the most recent
example coming in mind, there are many others).

Anyway, let's just switch pgstat_backend.c so as we use a static
PgStat_BackendPending (pending name) to store the pending IO stats
that could be reused for also the WAL bits. It does not seem that
complicated as far as I can see, removing pgstat_prep_backend_pending
and changing pgstat_flush_backend_entry_io to use the static pending
data. The patch set posted here is doing that within its
pgstat_flush_backend_entry_wal():
https://www.postgresql.org/message-id/Z4DrFhdlO6Xf13Xd@ip-10-97-1-34.eu-west-3.compute.internal

I could tweak that around the beginning of next week with a proposal
of patch. Bertrand, perhaps you'd prefer hack on this one?
--
Michael