From 39f3284fa8d69b7006554bdc58e1a4d87e89529d Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Tue, 20 Oct 2026 09:06:32 +0900 Subject: [PATCH v1] Don't free a cached RI plan that may be in use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, 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 under CLOBBER_FREED_MEMORY. 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 having ri_FetchPreparedPlan() only detach the stale plan from its hash entry, queueing it on a list that AtEOXact_RI() frees at end of transaction, where no RI check can be in progress. That is the same treatment the fast-path metadata already gets. The existing re-entrant FK check test covers this under CLOBBER_CACHE_ALWAYS or debug_discard_caches; a comment now says so. Reported-by: Tom Lane Discussion: https://postgr.es/m/3043944.1792163844@sss.pgh.pa.us Backpatch-through: 14 --- src/backend/utils/adt/ri_triggers.c | 77 ++++++++++++++++++----- src/test/regress/expected/foreign_key.out | 5 +- src/test/regress/sql/foreign_key.sql | 5 +- 3 files changed, 70 insertions(+), 17 deletions(-) diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 7143a900fe0..540febf21f7 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -231,6 +231,15 @@ 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, but possibly still being executed by an RI check further + * up the stack. Allocated in TopMemoryContext and released by + * AtEOXact_RI(), where no such execution can be in progress. See + * ri_FetchPreparedPlan(). + */ +static List *ri_plan_dead_list = NIL; + /* * Local function prototypes */ @@ -3689,12 +3698,27 @@ 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. + * + * We must not SPI_freeplan() it here, though. An RI check further up the + * stack may still be executing this very plan: the check's query can run + * user-defined code (a cast function applied to the FK values, say) whose + * DML fires another RI check for the same constraint, and hence the same + * query key, before the outer SPI_execute_snapshot() returns. If the + * plan has been invalidated in between, freeing it here would pull the + * SPIPlan out from under the outer _SPI_execute_plan(). So queue it for + * release at end of transaction instead, where no RI check can be in + * progress. */ entry->plan = NULL; if (plan) - SPI_freeplan(plan); + { + MemoryContext oldcxt = MemoryContextSwitchTo(TopMemoryContext); + + ri_plan_dead_list = lappend(ri_plan_dead_list, plan); + MemoryContextSwitchTo(oldcxt); + } return NULL; } @@ -3996,20 +4020,25 @@ 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(), queued on + * ri_plan_dead_list. + * + * 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) @@ -4021,4 +4050,22 @@ AtEOXact_RI(bool isCommit) ri_fpmeta_dead_list = dead->next_dead; pfree(dead); } + + 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 b1f36f6a1bb..cb9fcb9f62b 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -4035,7 +4035,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 99ea110e848..af6fb7800a4 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2993,7 +2993,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.47.3