On Mon, Mar 23, 2026 at 12:07 PM Zsolt Parragi <[email protected]> wrote: > > > I'm looking at this finalize at resowner part of this patch, and this > > maybe a stupid question, but: > > > > Why does the instrumentation need to be "finalized" on abort? If you run > > EXPLAIN ANALYZE and the query aborts, you don't get to see the stats anyway. > > The pg_session_buffer_usage in 0009 makes the information available, I > was able to see the issue with failing triggers with that. Even if > that part doesn't get committed in the end, a 3rd party extension > could still implement the same thing, and notice the missing > statistics. (And maybe it is useful to see some statistics about > failing queries?)
Right, there are basically two reasons we need the resource owner logic, or something equivalent: 1) To ensure the active stack entry gets reset correctly to where it was before, so that we don't corrupt it after an abort (if that's not done, we'll fail during regular regression tests) 2) To accumulate statistics to parent stack entries that did not participate in the abort As Zsolt noted, (2) matters for the pg_session_buffer_usage module, which is mainly intended to ensure that the logic we're adding keeps a top-level instrumentation available that includes the activity of aborted transactions/queries, since we're removing pgBufferUsage. It also matters for some edge cases in-tree today, e.g. procedures being tracked in pg_stat_statements. And I do see us being interested in tracking failed query activities in the future as Zsolt noted. --- FWIW, on the topic of resource owners and allocations, I've done a test over the weekend, and here is a question: It seems we could switch the Instrumentation allocations we're doing when inside a portal to PortalContext, and CurrentMemoryContext when outside a portal - instead of allocating in TopMemoryContext/TopTransactionContext. That works in practice, because resource owner cleanup happens before PortalContext cleanup, and simplifies the code a bit since we can skip copying into the current memory context (because the caller wants to be able to use the result after the finalize call). And if we leak we'd only leak until PortalContext gets cleaned up, instead of TopMemoryContext. To expand on that, in the previously posted v9 we have the following allocations: A) InstrStackState allocated under TopMemoryContext (long-lived, never freed) B) QueryInstrumentation allocated under TopMemoryContext (short-lived during query execution, explicitly freed up on abort or finalize call) C) NodeInstrumentation allocated under TopTransactionContext (short-lived during query execution, explicitly freed up on abort or finalize call) D) In other use cases, e.g. ANALYZE command that logs buffer usage, QueryInstrumentation allocated under TopMemoryContext (short-lived during command execution, explicitly freed up on abort or finalize call) And we could switch it instead to: A) InstrStackState allocated under TopMemoryContext (long-lived, never freed) B) QueryInstrumentation allocated under PortalContext (short-lived during query execution, *automatically* freed up on abort, manually on ExecutorEnd to avoid waiting for holdable cursors to free PortalContext) C) NodeInstrumentation allocated under PortalContext (short-lived during query execution, *automatically* freed up on abort, manually on ExecutorEnd to avoid waiting for holdable cursors to free PortalContext) D) In other use cases, e.g. ANALYZE command that logs buffer usage, QueryInstrumentation allocated under CurrentMemoryContext (short-lived during command execution, *automatically* freed up on abort and success case) However, this goes against the principle noted by Heikki over in [0] that ResOwners should use TopMemoryContext to avoid relying on the ordering of clean up operations. Thoughts? Thanks, Lukas [0]: https://www.postgresql.org/message-id/flat/a3197b31-f40d-4164-872d-906d8e9b374a%40iki.fi#526984c1be0662a7526bcfe0b358564b -- Lukas Fittl
