Re: per backend I/O statistics

On Mon, Jan 20, 2025 at 11:10:40AM +0000, Bertrand Drouvot wrote:
> 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

"static" and "dynamic" feel a bit off when used together.  For
"static", the stats are from a static state but they can be pushed to
the dshash, as well, which is in dynamic shared memory.

Hmm.  Thinking more about that, I think that we should leave the
existing "flush_pending_cb" alone.  For the two others, how about
"have_static_pending_cb" and "flush_static_cb" rather than "fixed"?
It seems important to me to show the relationship between these two.

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

Hmm.  I think that I'm OK with what you are doing here with the next
parts in mind, based on this argument.

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

Indeed.  I got this part slightly wrong here.  What you are doing with
an all-zero check looks simpler in the long run.
--
Michael