Hi Lukas, On Fri, Oct 30, 2026 at 12:19:51PM -0700, Lukas Fittl wrote: > On Fri, Oct 30, 2026 at 12:27 AM Bertrand Drouvot > <[email protected]> wrote: > > We could refactor it a bit by having InstrAccumParallelQuery take > arrays of WalUsage/BufferUsage, as well as a worker count argument, > and then save on the additions that way inside > InstrAccumParallelQuery?
Yeah that makes sense. That said, as this should be backpatched, we'd either have to change the function signature in the back branches or make master diverge from them. Neither option looks appealing. I'm not sure either is worth it given this is not performance critical though, so I'd vote to keep v1 as is in all branches. > > Also, worth to add some tests? > > Maybe? I'm always on the fence with how much tests we should be adding > for instrumentation fixes, because it needs a bunch of setup, can be > brittle in terms of comparing numbers (e Right, and a test relying on workers actually processing enough WAL work might also be fragile in the buildfarm. So I would not insist on adding one here. > FWIW, what I'd really wish for is a better way to guarantee the > invariant (i.e. nobody starts diffing pgWalUsage/pgBufferUsage and > emitting it into cumulative stats without taking care of this), but I > feel the only way to achieve that would be to make the globals private > to instrument.c and require the use of an appropriately named method > to access them, which doesn't seem worth it. Agreed. > Writing this out though makes me wonder though if we should add a > comment above pgBufferUsage/pgWalUsage in instrument.h warning of this > hazard, instead of just on the new globals below. +1, I think that should be enough for now. One comment about the patch. === 1 +extern PGDLLIMPORT BufferUsage pgBufferUsageFromWorkers; +extern PGDLLIMPORT WalUsage pgWalUsageFromWorkers; I wonder if those shouldn't remain private to instrument.c? Contrary to pgWalUsage, no callers need to access them directly, and they look more like internal such as save_pgWalUsage. So maybe they could be static and only ComputeOwnWalUsage() exposed? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
