Re: per backend I/O statistics

Hi,

On Mon, Jan 20, 2025 at 03:34:41PM +0900, Michael Paquier wrote:
> On Sat, Jan 18, 2025 at 05:53:31PM +0900, Michael Paquier wrote:
> > Hmm.  Such special complexities in pgstat.c are annoying.  There is
> > a stupid thing I am wondering here.  For the WAL stats, why couldn't
> > we place some calls of pgstat_prep_backend_pending() in strategic
> > places like XLogBeginInsert() to force all the allocation steps of the
> > pending entry to happen before we would enter the critical sections
> > when doing a WAL insertion?  As far as I can see, there is a special
> > case with 2PC where XLogBeginInsert() could be called in a critical
> > section, but that seems to be the only one at quick glance.
> 
> I've looked first at this idea over the week-end with a quick hack,
> and it did not finish well.

Yeah, that would probably also mean moving the current WAL pending increments to
the same "new" place (to be consistent) which does not seem great.

> And then it struck me that with a bit of redesign of the callbacks, so

Thanks! 

> as the existing flush_fixed_cb and have_fixed_pending_cb are usable
> with variable-numbered stats, we should be able to avoid the exception
> your first version of the patch has introduced.  Attached is a patch
> to achieve what I have in mind, which is more generic than your
> previous patch.  I've built my stuff as 0002 on top of your 0001.
>

Indeed. Another idea that I had would have been to create another callback,
but I prefer your idea.

=== 1

I think that flush_pending_cb, flush_cb and have_pending_cb are now somehow
confusing (one could think that have_pending_cb is only linked to
flush_pending_cb).

I think that it would be better to make the distinction based on "local/static"
vs "dynamic memory" pending stats instead: I did so in v3 attached, using:

.flush_dynamic_cb(): flushes pending entries tracked in dynamic memory
.flush_static_cb(): flushes pending stats from static/global variable

Also I reworded the comments a bit.

=== 2

-       /*
-        * Clear out the statistics buffer, so it can be re-used.
-        */
-       MemSet(&PendingBackendStats.pending_io, 0, sizeof(PgStat_PendingIO));

The inital intent was to clear *only" the pending IO stats, so that each
flush (IO, WAL once implemented,...) could clear the pending stats it is
responsible for.

=== 3

+       /*
+        * Clear out the statistics buffer, so it can be re-used.
+        */
+       MemSet(&PendingBackendStats, 0, sizeof(PgStat_BackendPending));

Not sure about this one, see above. I mean it is currently Ok but once we'll
introduce the WAL part then it will not be correct depending of the flag value
being passed.

So, I did put back the previous logic in place (setting to zero only the stats
the flush callback is responsible for) in v3 attached.

=== 4

+ * Backend statistics counts waiting to be flushed out.  We assume this variable
+ * inits to zeroes.  These counters may be reported within critical sections so
+ * we use static memory in order to avoid memory allocation.
+ */
+static PgStat_BackendPending PendingBackendStats = {0};

I now think we should memset to zero to avoid any padding issues (as more structs
will be added to PgStat_BackendPending). Oh and it's already done in
pgstat_create_backend(), so removing the {0} assignement in the attached.

=== 5

+       have_backendstats = false;

I don't think setting have_backendstats to false in pgstat_flush_backend()
is correct. I mean, it is correct currently but once we'll add the WAL part it
won't necessary be correct if the flags != PGSTAT_BACKEND_FLUSH_ALL. So, using a 
pg_memory_is_all_zeros check on PendingBackendStats instead in the attached.

> One key choice I have made is to hide PendingBackendStats within
> pgstat_backend.c so as it is possible to enforce some sanity checks
> there.

Yeah, better, thanks!

=== 6

In passing, I realized that:

"
 * Copyright (c) 2001-2025, PostgreSQL Global Development Group
"

in pgstat_backend.c is incorrect (fixing it in 0002).

PFA:

0001: which is full rebase
0002: the Copyright fix
.txt: the changes I've made on top of your 0002 (to not confuse the cfbot and
to ease your review).

Regards,

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