Hi Bertrand, On Fri, Oct 30, 2026 at 12:27 AM Bertrand Drouvot <[email protected]> wrote: > > In terms of how to fix this, I first tried to invent a mechanism that > > would keep instrumentation imported from workers separate in the > > leader (so we never double count in the global). But that turns out to > > be pretty tricky, because we have use cases (pg_stat_statements being > > the main one, but e.g. also EXPLAIN ANALYZE of a query calling a > > function that itself runs a parallel query) where we want to track > > whole-statement activity (leader + workers), including utility > > statements, and we want to be able to do so in a nested manner. > > Agreed. > > > Instead, at least for now, the easier solution seems to be to > > accumulate imported instrumentation data into both the existing > > pg(Wal|Buffer)Usage globals, and a separate set called > > pg(Wal|Buffer)UsageFromWorkers that counts only the activity from the > > workers. Any caller that wants to isolate the leader-only activity for > > reporting to the cumulative statistics system then subtracts the > > worker portion from the global instrumentation using a helper function > > (ComputeOwnWalUsage), and workers continue to report their own > > activity directly to the cumulative statistics system. > > Yeah that looks like the best way to proceed currently.
Great, thanks for thinking through it! > I wonder if the extra additions could be reduced though. What about first > accumulating all workers into a local BufferUsage/WalUsage and then calling > InstrAccumParallelQuery() once for all workers? Hmm. Not sure if the extra additions really matter (I don't think its a particularly performance critical code section), and I'm hesitant to have more duplicated code in the different call sites of InstrAccumParallelQuery. 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? We have this pattern of working on multiple instrumentation structs already in e.g. InstrAllocTrigger, so it doesn't seem too far fetched to do - and all callers of InstrAccumParallelQuery have a (currently duplicated) loop over the workers. > 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 (especially since producing a certain amount of WAL is involved), and more tests slow down the regression test suite. But happy to add it if we feel its worth it. 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. 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. Thanks, Lukas -- Lukas Fittl
