Hi Amit, Trakshan, Thanks both for the work on this. I've been iterating on Amit's v1 plus Trakshan's stack, and added error handling on top. Patch attached (v9).
Amit, does the approach used to defer freeing the plans look OK to you? To keep it conservative I don't free anything while the error is being handled; maybe that's not needed and we could free them in the PG_FINALLY instead. Regards, Lucas El jue, 22 oct 2026 a las 8:18, Amit Langote (<[email protected]>) escribió: > On Wed, Oct 21, 2026 at 9:02 PM Amit Langote <[email protected]> > wrote: > > Thanks for the patch. I'm going to study it and post an updated > > version tomorrow. I'd like to handle errors as well as Lucas's patch > > does but I'd stick to using PG_TRY in all the branches instead of > > resowner in some and PG_TRY in others. I believe subtransaction > > aborts are handled correctly by PG_TRY-based solution but I'd need to > > test that. In any case, I want to avoid hash table and resowner calls > > in the per-row path and keep the tracking code as minimal as possible. > > Sorry, ran out of time today, so will post the patch tomorrow. > > -- > Thanks, Amit Langote >
From 60f95ae2b0dcf6552ad4f2364e8439e29f8970ba Mon Sep 17 00:00:00 2001 From: Lucas Jeffrey <[email protected]> Date: Thu, 22 Oct 2026 17:10:43 -0300 Subject: [PATCH v9] Don't free a cached RI plan that may be in use ri_FetchPreparedPlan() freed a cached SPI plan as soon as it found it invalid, so that the caller could build and hash a replacement. But the plan may still be executing. For example, an RI check's query running user-defined code, such as a cast function applied to the FK values or a BEFORE trigger fired by an ON DELETE CASCADE on a self-referencing table, may perform DML that in turn fires another RI check for the same constraint before the outer SPI_execute_snapshot() returns. If the plan was invalidated in between, the inner check's ri_FetchPreparedPlan() frees it out from under the outer _SPI_execute_plan(), which then goes on to read the freed plan's plancache_list and crashes. CLOBBER_CACHE_ALWAYS guarantees the invalidation, which is how buildfarm member trilobite found this in the foreign_key test after 6aecabcd5e moved FK checks that need a cast function from the RI fast path to SPI. The bug itself is older than the fast path: it has been possible ever since ri_FetchPreparedPlan() started discarding invalid plans. Fix by keeping the plans being executed by ri_PerformCheck() on a small stack. When ri_FetchPreparedPlan() finds an invalid plan that no check up the stack is executing, it frees it right away as before; otherwise it only detaches it from its hash entry and queues it. Queued plans are freed as soon as no check is executing them anymore, when a check starts or finishes. The stack is popped on error too, but nothing is freed while the error is being handled: what an erroring check leaves queued is freed by the next check, or by AtEOXact_RI() at the latest. The existing re-entrant FK check test covers this under CLOBBER_CACHE_ALWAYS or debug_discard_caches; a comment now says so. --- src/backend/utils/adt/ri_triggers.c | 147 +++++++++++++++++++--- src/test/regress/expected/foreign_key.out | 5 +- src/test/regress/sql/foreign_key.sql | 5 +- 3 files changed, 136 insertions(+), 21 deletions(-) diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index c3ea7db72ff..d5791d419d1 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -247,6 +247,53 @@ static dclist_head ri_constraint_cache_valid_list; */ static FastPathMeta *ri_fpmeta_dead_list = NULL; +/* + * SPI plans found invalid by ri_FetchPreparedPlan() and removed from + * ri_query_cache while an RI check further up the stack was still executing + * them. Allocated in TopMemoryContext. ri_PerformCheck() releases them + * once no check is executing them anymore; what an erroring check leaves + * behind is released by AtEOXact_RI(). See ri_FetchPreparedPlan(). + */ +static List *ri_plan_dead_list = NIL; + +/* + * Plans being executed by ri_PerformCheck(), innermost last. Entries are + * popped on error too, so the stack is empty at end of transaction. + */ +static SPIPlanPtr *ri_exec_stack = NULL; +static int ri_exec_stack_depth = 0; +static int ri_exec_stack_size = 0; + +static bool +ri_PlanIsExecuting(SPIPlanPtr plan) +{ + for (int i = 0; i < ri_exec_stack_depth; i++) + if (ri_exec_stack[i] == plan) + return true; + return false; +} + +static void +ri_FreeDeadPlans(void) +{ + List *keep = NIL; + ListCell *lc; + MemoryContext oldcxt = MemoryContextSwitchTo(TopMemoryContext); + + foreach(lc, ri_plan_dead_list) + { + SPIPlanPtr plan = (SPIPlanPtr) lfirst(lc); + + if (ri_PlanIsExecuting(plan)) + keep = lappend(keep, plan); + else + SPI_freeplan(plan); + } + list_free(ri_plan_dead_list); + ri_plan_dead_list = keep; + MemoryContextSwitchTo(oldcxt); +} + /* * Local function prototypes */ @@ -2727,10 +2774,37 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, * the outer query's after-trigger context and fire after all RI updates * on the same row are complete, rather than immediately. */ - spi_result = SPI_execute_snapshot(qplan, - vals, nulls, - test_snapshot, crosscheck_snapshot, - false, false, limit); + if (ri_exec_stack_depth >= ri_exec_stack_size) + { + ri_exec_stack_size = Max(8, ri_exec_stack_size * 2); + if (ri_exec_stack == NULL) + ri_exec_stack = MemoryContextAlloc(TopMemoryContext, + ri_exec_stack_size * sizeof(SPIPlanPtr)); + else + ri_exec_stack = repalloc(ri_exec_stack, + ri_exec_stack_size * sizeof(SPIPlanPtr)); + } + + /* Release what a check that ended in an error left queued. */ + if (ri_plan_dead_list != NIL) + ri_FreeDeadPlans(); + ri_exec_stack[ri_exec_stack_depth++] = qplan; + PG_TRY(); + { + spi_result = SPI_execute_snapshot(qplan, + vals, nulls, + test_snapshot, crosscheck_snapshot, + false, false, limit); + } + PG_FINALLY(); + { + ri_exec_stack_depth--; + } + PG_END_TRY(); + + /* Not done on error: no freeing plans in the exception path. */ + if (ri_plan_dead_list != NIL) + ri_FreeDeadPlans(); /* Restore UID and security context */ SetUserIdAndSecContext(save_userid, save_sec_context); @@ -3697,12 +3771,21 @@ ri_FetchPreparedPlan(RI_QueryKey *key) return plan; /* - * Otherwise we might as well flush the cached plan now, to free a little - * memory space before we make a new one. + * Otherwise, detach the stale plan from the entry so that the caller's + * replacement can be hashed under the same key. Free it only if no RI + * check further up the stack is still executing it; otherwise queue it, + * and ri_PerformCheck() frees it once that execution finishes. */ entry->plan = NULL; - if (plan) + if (plan && !ri_PlanIsExecuting(plan)) SPI_freeplan(plan); + else if (plan) + { + MemoryContext oldcxt = MemoryContextSwitchTo(TopMemoryContext); + + ri_plan_dead_list = lappend(ri_plan_dead_list, plan); + MemoryContextSwitchTo(oldcxt); + } return NULL; } @@ -4014,20 +4097,27 @@ RI_FKey_trigger_type(Oid tgfoid) * AtEOXact_RI * End-of-transaction cleanup for referential integrity. * - * Currently this only releases fast-path metadata detached during the - * transaction. InvalidateConstraintCacheCallBack() cannot free a - * FastPathMeta when it detaches one, because an RI check further up the - * stack may still hold a pointer into it. It queues them on - * ri_fpmeta_dead_list instead, and we release them here, where no such - * reference can exist. isCommit is accepted for consistency with the - * other AtEOXact_* routines but is not used: the release is the same on - * the commit and the abort path. + * This releases objects that were detached from the RI caches during the + * transaction but could not be freed on the spot, because an RI check + * further up the stack might still have been using them: + * + * - fast-path metadata detached by InvalidateConstraintCacheCallBack(), + * queued on ri_fpmeta_dead_list; + * + * - SPI plans found invalid by ri_FetchPreparedPlan() while being executed, + * queued on ri_plan_dead_list. ri_PerformCheck() normally frees them as + * soon as the execution finishes; only a check that ended in an error + * leaves them for us. + * + * Here no RI check can be in progress, so both can be released. isCommit + * is accepted for consistency with the other AtEOXact_* routines but is + * not used: the release is the same on the commit and the abort path. * * There is no AtEOSubXact_RI() counterpart. Nothing here is scoped to a - * subtransaction: a detached FastPathMeta stays reachable from the dead - * list whichever subtransaction detached it, and a check holding a pointer - * into one may be running at an outer level, so releasing at subtransaction - * end would be unsafe as well as unnecessary. + * subtransaction: a detached object stays reachable from its dead list + * whichever subtransaction detached it, and a check using one may be + * running at an outer level, so releasing at subtransaction end would be + * unsafe as well as unnecessary. */ void AtEOXact_RI(bool isCommit) @@ -4040,4 +4130,23 @@ AtEOXact_RI(bool isCommit) MemoryContextDelete(dead->scratch_cxt); pfree(dead); } + + Assert(ri_exec_stack_depth == 0); + if (ri_plan_dead_list != NIL) + { + List *plans = ri_plan_dead_list; + ListCell *lc; + + /* + * Clear the static before freeing, so that an error while freeing a + * plan (unexpected, but we may be on the abort path already) does not + * leave a dangling pointer to a half-freed plan in the list. + */ + ri_plan_dead_list = NIL; + + foreach(lc, plans) + SPI_freeplan((SPIPlanPtr) lfirst(lc)); + + list_free(plans); + } } diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index df5e827d57e..2185cb5329c 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -4042,7 +4042,10 @@ END$$; CREATE CAST (fp_vch AS int) WITH FUNCTION fp_vcast(fp_vch) AS IMPLICIT; CREATE TABLE fp_reentry_fk (a fp_vch REFERENCES fp_reentry_pk (id)); --- cast re-enters with DML on the same FK +-- The cast re-enters with DML on the same FK, so the inner check looks up +-- the same cached SPI plan while the outer check is still executing it. +-- If the plan has been invalidated meanwhile (as CLOBBER_CACHE_ALWAYS makes +-- certain), the inner check must not free it out from under the outer one. INSERT INTO fp_reentry_fk SELECT row(1)::fp_vch FROM generate_series(1, 2); SELECT a, count(*) FROM fp_reentry_fk GROUP BY a ORDER BY a; a | count diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index ce35c465b81..d12feee650b 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -3002,7 +3002,10 @@ END$$; CREATE CAST (fp_vch AS int) WITH FUNCTION fp_vcast(fp_vch) AS IMPLICIT; CREATE TABLE fp_reentry_fk (a fp_vch REFERENCES fp_reentry_pk (id)); --- cast re-enters with DML on the same FK +-- The cast re-enters with DML on the same FK, so the inner check looks up +-- the same cached SPI plan while the outer check is still executing it. +-- If the plan has been invalidated meanwhile (as CLOBBER_CACHE_ALWAYS makes +-- certain), the inner check must not free it out from under the outer one. INSERT INTO fp_reentry_fk SELECT row(1)::fp_vch FROM generate_series(1, 2); SELECT a, count(*) FROM fp_reentry_fk GROUP BY a ORDER BY a; DROP TABLE fp_reentry_fk, fp_reentry_pk; -- 2.34.1
