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
signature.asc
Description: PGP signature
