HashAgg/subplan spill contexts (table_ctx/tmp_ctx) skip the no-drop-arena teardown callback used elsewhere, leaking on query abort

[MauricioPerera]

Summary

mcx::MemoryContext's parent-child hierarchy is accounting-only, not an ownership/cascade tree (confirmed by the crate's own test, child_may_outlive_parent_accounting_safely, crates/_support/mcx/src/tests.rs:219-230). A MemoryContext value has a real Drop impl (crates/_support/mcx/src/lib.rs:1358-1368) that tears down its own backing arena — but when a MemoryContext is itself an occupant of a no-drop bump arena (Backend::Bump/Backend::BumpForget, as opposed to BumpDrop), that outer arena's reset/drop does not run occupant destructors at all unless one is explicitly registered via MemoryContext::register_reset_callback(|| unsafe { drop_in_place(...) }).

This codebase has an established, correctly-applied fix for exactly this situation: make_agg_state_node (crates/backend/executor/nodeagg/src/lib.rs:801-816) explicitly registers such a callback for AggStateNode (which itself holds a MemoryContext), with the comment:

// The node's MemoryContext is droppy inside a no-drop arena: the query
// context's reset callback is its destructor (docs/no-drop.md guard rule).
// SAFETY: fires exactly once, before the arena bytes are reclaimed.

Several sibling MemoryContext-holding structs in the same crate/module family do not get this treatment:

• PerHashData.table_ctx / PerHashData.tmp_ctx (crates/backend/executor/nodeagg/src/lib.rs:713, created via mcx.context().new_child_bump(...) at lib.rs:2104-2105) — bare MemoryContext fields, no registered callback.
• gsets.rs:621 — same tmp_ctx pattern for grouping-sets hash spill.
• crates/backend/executor/execmain/src/nodesubplan.rs:653-654 — table_ctx/hashtempcxt of HashedSubPlanState, same unprotected pattern.
• crates/backend/executor/execmain/src/nodesubplan.rs:487 — array_ctx (ARRAY_SUBLINK only), same pattern.
• crates/backend/executor/execmain/src/lanev2.rs:13184 — stage_cxt in StagedFoldAggSink.

On the normal completion path this isn't an issue: e.g. exec_end_agg explicitly destructures AggStateData { perhash, .. } (nodeagg/src/lib.rs:2675), which moves perhash/table_ctx onto the stack as ordinary Rust values, so MemoryContext::drop runs normally through standard Rust drop semantics.

The gap is on the abort/error path: if an error unwinds through query execution without reaching exec_end_agg (the standard way memory contexts are supposed to make cleanup "free" on transaction abort — the query context is expected to be reset/discarded wholesale, no manual teardown required), and if the enclosing AggStateData/HashedSubPlanState/StagedFoldAggSink value lives inside a no-drop bump arena (as AggStateNode demonstrably does, per the comment above), then resetting that outer arena does not invoke table_ctx's/tmp_ctx's/hashtempcxt's/array_ctx's/stage_cxt's own Drop::drop, so their backing bump arenas (which can be sized proportional to spilled hash-agg/subplan-hash-table data — potentially large) are never reclaimed for the lifetime of the enclosing no-drop arena (typically the query/statement context or longer).

Why this matters

This is a resource-leak class bug, not a crash or memory-safety violation — but it's a real, silent leak whose size scales with the amount of data spilled into a hash aggregate or hashed subplan at the time of an aborted query, and it would compound under any workload that aborts hash-aggregate or IN/EXISTS-subplan queries repeatedly (e.g. statement_timeout hits, explicit ROLLBACK, constraint violations mid-query, client cancel).

Suggested scope for a fix

Apply the same register_reset_callback(|| unsafe { drop_in_place(...) }) idiom used in make_agg_state_node (nodeagg/src/lib.rs:801-816) — or the equivalent NonNull<MemoryContext> + explicit teardown pattern used in make_table_ctx/make_arg_ctx/make_table_context/create_expr_context elsewhere in the executor tree — to each of the five call sites listed above, so their MemoryContext fields get torn down correctly even when the abort path never reaches the normal exec_end_*/rescan teardown functions.

Environment / where found

Found via static source review of a fresh clone of this repo, as part of a broader review of the mcx memory-context crate (which is otherwise a notably strong, borrow-checker-enforced design — see the reset()-vs-live-allocation compile_fail doctests in crates/_support/mcx/src/lib.rs:1447-1462). Have not attempted to build/run pgrust myself or reproduce the leak at runtime (e.g. via an actual aborted hash-agg query with memory accounting before/after); this report is based on reading crates/_support/mcx/src/lib.rs, crates/backend/executor/nodeagg/src/lib.rs, crates/backend/executor/nodeagg/src/gsets.rs, crates/backend/executor/execmain/src/nodesubplan.rs, and crates/backend/executor/execmain/src/lanev2.rs directly, and comparing the unprotected sites against the codebase's own established fix pattern for the identical class of issue in the same file (agg_node).