From add36dd0f109ea1ccd57a2c0a1ea84bae2e263cb Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Wed, 19 Aug 2026 21:58:55 +0900 Subject: [PATCH v3 2/3] Track RI fast-path FK-check batches per firing cycle Commit 34a30786293 fixed an RI fast-path crash under nested C-level SPI by keeping batch-callback lists per after-trigger query depth. That fix was incomplete: the RI fast path still tracked callback registration with one global flag. Once an outer firing cycle had registered its callback, the flag suppressed registration for a nested cycle, leaving the nested batch to be handled by the outer callback, too late and with the wrong snapshot, potentially after the ResourceOwner holding its relations had gone away. Nor is per-depth callback registration sufficient while the cache is keyed only by constraint OID. If nested firing checks the same constraint, it reuses the outer entry, combining rows that must be checked in separate firing cycles. Key the cache by both constraint OID and query depth. Register a callback for each depth that creates an entry, and make ri_FastPathEndBatch() flush and release only entries belonging to the ending depth. Add AfterTriggerCurrentQueryDepth() so ri_triggers.c can obtain the current depth; depth -1 represents deferred firing. Add regression coverage for nested firing through a cursor portal, whose resources must not outlive the nested cycle, and for deferred firing at query depth -1. Reported-by: Noah Misch Reported-by: Peter Geoghegan Discussion: https://postgr.es/m/20260705222115.be.noahmisch@microsoft.com Discussion: https://postgr.es/m/CAH2-Wz=D533JbF_ak_Pc8kP0FKse-ju8DnMxtjvY==yHsP4xgw@mail.gmail.com Backpatch-through: 19 --- src/backend/commands/trigger.c | 14 +++ src/backend/utils/adt/ri_triggers.c | 114 +++++++++++++++++----- src/include/commands/trigger.h | 1 + src/test/regress/expected/foreign_key.out | 63 ++++++++++++ src/test/regress/sql/foreign_key.sql | 54 ++++++++++ src/tools/pgindent/typedefs.list | 1 + 6 files changed, 224 insertions(+), 23 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 11941b5f5c3..39a1576e75d 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -6967,3 +6967,17 @@ AfterTriggerIsActive(void) { return afterTriggers.firing_depth > 0; } + +/* + * AfterTriggerCurrentQueryDepth + * Return the current after-trigger query nesting depth. + * + * Lets a batch-callback registrant (e.g. the RI fast path) associate cached + * state with the firing cycle that created it, so a nested cycle's callback + * acts only on its own entries. Returns -1 outside any query level. + */ +int +AfterTriggerCurrentQueryDepth(void) +{ + return afterTriggers.query_depth; +} diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 58cbc4d3b5e..6863ebb8bce 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -231,13 +231,27 @@ typedef struct RI_CompareHashEntry */ #define RI_FASTPATH_BATCH_SIZE 64 +/* + * RI_FastPathKey + * Hash key for an RI_FastPathEntry. + * + * A constraint can be checked in nested trigger-firing cycles. Each cycle + * must have a separate entry so that its rows are checked with that cycle's + * snapshot and its resources are released by that cycle's callback. + */ +typedef struct RI_FastPathKey +{ + Oid conoid; /* pg_constraint OID */ + int query_depth; /* after-trigger query depth */ +} RI_FastPathKey; + /* * RI_FastPathEntry - * Per-constraint cache of resources needed by ri_FastPathBatchFlush(). + * Per-constraint, per-firing-cycle cache of resources needed by + * ri_FastPathBatchFlush(). * - * One entry per constraint, keyed by pg_constraint OID. Created lazily - * by ri_FastPathGetEntry() on first use within a trigger-firing batch - * and torn down by ri_FastPathTeardown() at batch end. + * Created lazily by ri_FastPathGetEntry() on first use within a + * trigger-firing batch and torn down by ri_FastPathTeardown() at batch end. * * FK tuples are buffered in batch[] across trigger invocations and * flushed when the buffer fills or the batch ends. @@ -251,7 +265,7 @@ typedef struct RI_CompareHashEntry */ typedef struct RI_FastPathEntry { - Oid conoid; /* hash key: pg_constraint OID */ + RI_FastPathKey key; /* hash key */ Oid fk_relid; /* for ri_FastPathEndBatch() */ Relation pk_rel; Relation idx_rel; @@ -284,7 +298,6 @@ static HTAB *ri_compare_cache = NULL; static dclist_head ri_constraint_cache_valid_list; static HTAB *ri_fastpath_cache = NULL; -static bool ri_fastpath_callback_registered = false; static bool ri_fastpath_flushing = false; /* @@ -382,7 +395,7 @@ pg_noreturn static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, static RI_FastPathEntry *ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel); static void ri_FastPathEndBatch(void *arg); -static void ri_FastPathTeardown(void); +static void ri_FastPathTeardown(int depth); /* @@ -4302,6 +4315,7 @@ ri_FastPathEndBatch(void *arg) { HASH_SEQ_STATUS status; RI_FastPathEntry *entry; + int my_depth = (int) (intptr_t) arg; if (ri_fastpath_cache == NULL) return; @@ -4326,10 +4340,13 @@ ri_FastPathEndBatch(void *arg) hash_seq_init(&status, ri_fastpath_cache); while ((entry = hash_seq_search(&status)) != NULL) { - if (entry->batch_count > 0) + /* Flush only entries created in the cycle now ending. */ + if (entry->key.query_depth == my_depth && entry->batch_count > 0) { Relation fk_rel = table_open(entry->fk_relid, AccessShareLock); - RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid); + RI_ConstraintInfo *riinfo; + + riinfo = ri_LoadConstraintInfo(entry->key.conoid); ri_FastPathBatchFlush(entry, fk_rel, riinfo); table_close(fk_rel, NoLock); @@ -4342,17 +4359,26 @@ ri_FastPathEndBatch(void *arg) } PG_END_TRY(); - ri_FastPathTeardown(); + /* + * Release this cycle's entries and remove them from the cache; leave + * outer cycles' entries for their own callbacks. Destroy the cache once + * empty. + */ + ri_FastPathTeardown(my_depth); } /* * ri_FastPathTeardown - * Tear down all cached fast-path state. + * Release and remove the cached entries of one firing cycle, and drop + * the cache once it holds no more entries. * - * Called from ri_FastPathEndBatch() after flushing any remaining rows. + * Called from ri_FastPathEndBatch() with the depth of the cycle that is + * ending: it releases only that cycle's entries, leaving an outer cycle's + * still-live entries for their own callbacks. The cache (and its static + * pointer) go away once the last entry is removed. */ static void -ri_FastPathTeardown(void) +ri_FastPathTeardown(int depth) { HASH_SEQ_STATUS status; RI_FastPathEntry *entry; @@ -4363,6 +4389,8 @@ ri_FastPathTeardown(void) hash_seq_init(&status, ri_fastpath_cache); while ((entry = hash_seq_search(&status)) != NULL) { + if (entry->key.query_depth != depth) + continue; if (entry->idx_rel) index_close(entry->idx_rel, NoLock); if (entry->pk_rel) @@ -4373,11 +4401,15 @@ ri_FastPathTeardown(void) ExecDropSingleTupleTableSlot(entry->fk_slot); if (entry->flush_cxt) MemoryContextDelete(entry->flush_cxt); + hash_search(ri_fastpath_cache, &entry->key, HASH_REMOVE, NULL); } - hash_destroy(ri_fastpath_cache); - ri_fastpath_cache = NULL; - ri_fastpath_callback_registered = false; + if (hash_get_num_entries(ri_fastpath_cache) == 0) + { + hash_destroy(ri_fastpath_cache); + ri_fastpath_cache = NULL; + ri_fastpath_flushing = false; + } } /* @@ -4423,7 +4455,6 @@ AtEOXact_RI(bool isCommit) * memory-context reset; here we only drop the references to it. */ ri_fastpath_cache = NULL; - ri_fastpath_callback_registered = false; /* * Also clear the in-flush flag. ri_FastPathEndBatch() already clears it @@ -4462,15 +4493,20 @@ AtEOXact_RI(bool isCommit) static RI_FastPathEntry * ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) { + RI_FastPathKey key; RI_FastPathEntry *entry; bool found; + int cur_depth = AfterTriggerCurrentQueryDepth(); + + key.conoid = riinfo->constraint_id; + key.query_depth = cur_depth; /* Create hash table on first use in this batch */ if (ri_fastpath_cache == NULL) { HASHCTL ctl; - ctl.keysize = sizeof(Oid); + ctl.keysize = sizeof(RI_FastPathKey); ctl.entrysize = sizeof(RI_FastPathEntry); ctl.hcxt = TopTransactionContext; ri_fastpath_cache = hash_create("RI fast-path cache", @@ -4479,7 +4515,7 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) HASH_ELEM | HASH_BLOBS | HASH_CONTEXT); } - entry = hash_search(ri_fastpath_cache, &riinfo->constraint_id, + entry = hash_search(ri_fastpath_cache, &key, HASH_ENTER, &found); if (!found) @@ -4536,11 +4572,43 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) ALLOCSET_SMALL_SIZES); MemoryContextSwitchTo(oldcxt); - /* Ensure cleanup at end of this trigger-firing batch */ - if (!ri_fastpath_callback_registered) + /* + * Ensure ri_FastPathEndBatch() is registered for THIS firing cycle. + * RegisterAfterTriggerBatchCallback() appends to the current + * after-trigger query depth's callback list and fires at that depth's + * AfterTriggerEndQuery(), so a nested cycle needs its own + * registration; a single global latch would leave the nested cycle's + * list empty and its batch unflushed. Pass the depth as the callback + * arg so the callback flushes only its own cycle's entries. + */ { - RegisterAfterTriggerBatchCallback(ri_FastPathEndBatch, NULL); - ri_fastpath_callback_registered = true; + bool depth_registered = false; + HASH_SEQ_STATUS reg_status; + RI_FastPathEntry *other; + + /* + * Register the callback once per firing cycle (query depth), + * including the deferred cycle at depth -1. Rather than track + * registered depths separately, check whether any other cache + * entry was already created at this depth: if so, its creation + * already registered the callback for this cycle. (The + * just-created entry is already in the hash, so skip it by + * pointer.) + */ + hash_seq_init(®_status, ri_fastpath_cache); + while ((other = hash_seq_search(®_status)) != NULL) + { + if (other != entry && other->key.query_depth == cur_depth) + { + depth_registered = true; + hash_seq_term(®_status); + break; + } + } + + if (!depth_registered) + RegisterAfterTriggerBatchCallback(ri_FastPathEndBatch, + (void *) (intptr_t) cur_depth); } entry->flushing = false; diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 0c3d485abf4..1f268f87957 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -309,6 +309,7 @@ typedef void (*AfterTriggerBatchCallback) (void *arg); extern void RegisterAfterTriggerBatchCallback(AfterTriggerBatchCallback callback, void *arg); extern bool AfterTriggerIsActive(void); +extern int AfterTriggerCurrentQueryDepth(void); extern void AtEOXact_RI(bool isCommit); diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 01343c58e11..d7116084d8f 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -3951,3 +3951,66 @@ ROLLBACK; RESET ROLE; DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk; DROP ROLE regress_fpav_role; +-- An AFTER trigger runs a query of its own, and that query inserts into a +-- second table with a fast-path foreign key. The entry the nested INSERT +-- creates belongs to the cursor's portal, which is gone by the time the +-- entry is torn down at the end of the outer statement. Every key stored +-- below is present in its referenced table, so the INSERT must just succeed. +CREATE TABLE fp_customer (id int PRIMARY KEY); +INSERT INTO fp_customer VALUES (1); +CREATE TABLE fp_product (id int PRIMARY KEY); +INSERT INTO fp_product SELECT generate_series(1, 4); +CREATE TABLE fp_kit_component (kit_product_id int, component_product_id int); +INSERT INTO fp_kit_component VALUES (1, 2), (1, 3), (1, 4); +CREATE TABLE fp_order (id int, customer_id int REFERENCES fp_customer, + product_id int); +CREATE TABLE fp_order_item (order_id int, product_id int + REFERENCES fp_product); +CREATE FUNCTION fp_add_order_item(order_id int, product_id int) RETURNS int + LANGUAGE plpgsql AS $$ +BEGIN + INSERT INTO fp_order_item VALUES (order_id, product_id); + RETURN product_id; +END$$; +CREATE FUNCTION fp_expand_kit() RETURNS trigger LANGUAGE plpgsql AS $$ +DECLARE + component_id int; + ncomponents int := 0; +BEGIN + FOR component_id IN + SELECT fp_add_order_item(NEW.id, component_product_id) + FROM fp_kit_component WHERE kit_product_id = NEW.product_id + LOOP + ncomponents := ncomponents + 1; + END LOOP; + RAISE NOTICE 'order % expanded into % order items', NEW.id, ncomponents; + RETURN NULL; +END$$; +CREATE TRIGGER fp_expand_kit_trg AFTER INSERT ON fp_order + FOR EACH ROW EXECUTE FUNCTION fp_expand_kit(); +INSERT INTO fp_order VALUES (1, 1, 1); +NOTICE: order 1 expanded into 3 order items +SELECT count(*) FROM fp_order_item; + count +------- + 3 +(1 row) + +DROP TABLE fp_order, fp_order_item, fp_kit_component, fp_product, fp_customer; +DROP FUNCTION fp_expand_kit(), fp_add_order_item(int, int); +-- Deferred FK check fires at commit (query depth -1); its batch must still get +-- a callback registered and flushed. +CREATE TABLE fp_deferred_pk (id int PRIMARY KEY); +CREATE TABLE fp_deferred_fk (a int REFERENCES fp_deferred_pk (id) + DEFERRABLE INITIALLY DEFERRED); +BEGIN; +INSERT INTO fp_deferred_fk VALUES (1); +INSERT INTO fp_deferred_pk VALUES (1); +COMMIT; +SELECT count(*) AS deferred_rows FROM fp_deferred_fk; -- 1, check passed at commit + deferred_rows +--------------- + 1 +(1 row) + +DROP TABLE fp_deferred_fk, fp_deferred_pk; diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 987cea61ba2..acd4fa8dea5 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2897,3 +2897,57 @@ ROLLBACK; RESET ROLE; DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk; DROP ROLE regress_fpav_role; + +-- An AFTER trigger runs a query of its own, and that query inserts into a +-- second table with a fast-path foreign key. The entry the nested INSERT +-- creates belongs to the cursor's portal, which is gone by the time the +-- entry is torn down at the end of the outer statement. Every key stored +-- below is present in its referenced table, so the INSERT must just succeed. +CREATE TABLE fp_customer (id int PRIMARY KEY); +INSERT INTO fp_customer VALUES (1); +CREATE TABLE fp_product (id int PRIMARY KEY); +INSERT INTO fp_product SELECT generate_series(1, 4); +CREATE TABLE fp_kit_component (kit_product_id int, component_product_id int); +INSERT INTO fp_kit_component VALUES (1, 2), (1, 3), (1, 4); +CREATE TABLE fp_order (id int, customer_id int REFERENCES fp_customer, + product_id int); +CREATE TABLE fp_order_item (order_id int, product_id int + REFERENCES fp_product); +CREATE FUNCTION fp_add_order_item(order_id int, product_id int) RETURNS int + LANGUAGE plpgsql AS $$ +BEGIN + INSERT INTO fp_order_item VALUES (order_id, product_id); + RETURN product_id; +END$$; +CREATE FUNCTION fp_expand_kit() RETURNS trigger LANGUAGE plpgsql AS $$ +DECLARE + component_id int; + ncomponents int := 0; +BEGIN + FOR component_id IN + SELECT fp_add_order_item(NEW.id, component_product_id) + FROM fp_kit_component WHERE kit_product_id = NEW.product_id + LOOP + ncomponents := ncomponents + 1; + END LOOP; + RAISE NOTICE 'order % expanded into % order items', NEW.id, ncomponents; + RETURN NULL; +END$$; +CREATE TRIGGER fp_expand_kit_trg AFTER INSERT ON fp_order + FOR EACH ROW EXECUTE FUNCTION fp_expand_kit(); +INSERT INTO fp_order VALUES (1, 1, 1); +SELECT count(*) FROM fp_order_item; +DROP TABLE fp_order, fp_order_item, fp_kit_component, fp_product, fp_customer; +DROP FUNCTION fp_expand_kit(), fp_add_order_item(int, int); + +-- Deferred FK check fires at commit (query depth -1); its batch must still get +-- a callback registered and flushed. +CREATE TABLE fp_deferred_pk (id int PRIMARY KEY); +CREATE TABLE fp_deferred_fk (a int REFERENCES fp_deferred_pk (id) + DEFERRABLE INITIALLY DEFERRED); +BEGIN; +INSERT INTO fp_deferred_fk VALUES (1); +INSERT INTO fp_deferred_pk VALUES (1); +COMMIT; +SELECT count(*) AS deferred_rows FROM fp_deferred_fk; -- 1, check passed at commit +DROP TABLE fp_deferred_fk, fp_deferred_pk; diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 298a3d586e7..d7e289b3867 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2530,6 +2530,7 @@ RI_CompareHashEntry RI_CompareKey RI_ConstraintInfo RI_FastPathEntry +RI_FastPathKey RI_QueryHashEntry RI_QueryKey RTEKind -- 2.47.3