diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 917a544c32b..06e7728c45d 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -228,8 +228,8 @@ typedef struct RI_CompareHashEntry * relations are held open with locks for the transaction duration, preventing * relcache invalidation. The entry itself is torn down at batch end by * ri_FastPathEndBatch(); on abort, ResourceOwner releases the cached - * relations and the XactCallback/SubXactCallback NULL the static cache pointer - * to prevent any subsequent access. + * relations and the XactCallback NULLs the static cache pointer to prevent + * any subsequent access. */ typedef struct RI_FastPathEntry { @@ -267,6 +267,7 @@ 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; /* * Local function prototypes @@ -469,14 +470,30 @@ RI_FKey_check(TriggerData *trigdata) */ if (ri_fastpath_is_applicable(riinfo)) { - if (AfterTriggerIsActive()) + if (AfterTriggerIsActive() && + GetCurrentTransactionNestLevel() == 1 && + !ri_fastpath_flushing) { /* Batched path: buffer and probe in groups */ ri_FastPathBatchAdd(riinfo, fk_rel, newslot); } else { - /* ALTER TABLE validation: per-row, no cache */ + /* + * Per-row path, used when batching is not safe or not applicable: + * + * - ALTER TABLE validation, where no after-trigger firing is + * active; + * + * - any FK check inside a subtransaction, since the batch cache + * is confined to the top transaction level (it cannot be cleanly + * unwound on subxact abort); + * + * - a re-entrant check from user cast/operator code running + * during a batch flush, since adding a cache entry while + * ri_FastPathEndBatch is iterating the cache could leave it + * unflushed. + */ ri_FastPathCheck(riinfo, fk_rel, newslot); } return PointerGetDatum(NULL); @@ -4181,19 +4198,41 @@ ri_FastPathEndBatch(void *arg) if (ri_fastpath_cache == NULL) return; - /* Flush any partial batches -- can throw ERROR */ - hash_seq_init(&status, ri_fastpath_cache); - while ((entry = hash_seq_search(&status)) != NULL) + /* + * Set a flag for the duration of the scan so that any FK check triggered + * by user cast or operator code during a flush takes the per-row path + * instead of adding a new entry to the cache we are iterating. A new + * entry could land in an already-scanned bucket and then be torn down + * unflushed below. + * + * The flush can throw ERROR (a reported constraint violation, or an error + * from the user code it runs). In that case ri_FastPathTeardown below is + * skipped; the ResourceOwner and the transaction-end callback handle + * resource cleanup on the abort path. The PG_FINALLY only resets the + * flag and deliberately does not attempt teardown. + */ + Assert(!ri_fastpath_flushing); + ri_fastpath_flushing = true; + PG_TRY(); { - if (entry->batch_count > 0) + hash_seq_init(&status, ri_fastpath_cache); + while ((entry = hash_seq_search(&status)) != NULL) { - Relation fk_rel = table_open(entry->fk_relid, AccessShareLock); - RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid); + if (entry->batch_count > 0) + { + Relation fk_rel = table_open(entry->fk_relid, AccessShareLock); + RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid); - ri_FastPathBatchFlush(entry, fk_rel, riinfo); - table_close(fk_rel, NoLock); + ri_FastPathBatchFlush(entry, fk_rel, riinfo); + table_close(fk_rel, NoLock); + } } } + PG_FINALLY(); + { + ri_fastpath_flushing = false; + } + PG_END_TRY(); ri_FastPathTeardown(); } @@ -4245,22 +4284,14 @@ ri_FastPathXactCallback(XactEvent event, void *arg) */ ri_fastpath_cache = NULL; ri_fastpath_callback_registered = false; -} -static void -ri_FastPathSubXactCallback(SubXactEvent event, SubTransactionId mySubid, - SubTransactionId parentSubid, void *arg) -{ - if (event == SUBXACT_EVENT_ABORT_SUB) - { - /* - * ResourceOwner already released relations. NULL the static pointers - * so the still-registered batch callback becomes a no-op for the rest - * of this transaction. - */ - ri_fastpath_cache = NULL; - ri_fastpath_callback_registered = false; - } + /* + * Also clear the in-flush flag. ri_FastPathEndBatch() already clears it + * via PG_FINALLY, so this is just defensive: it keeps a stale flag from + * surviving into the next transaction should any future path leave it + * set. + */ + ri_fastpath_flushing = false; } /* @@ -4287,7 +4318,6 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) if (!ri_fastpath_xact_callback_registered) { RegisterXactCallback(ri_FastPathXactCallback, NULL); - RegisterSubXactCallback(ri_FastPathSubXactCallback, NULL); ri_fastpath_xact_callback_registered = true; } diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index e08dff99f03..e1563144d4c 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -3768,3 +3768,27 @@ SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1 (1 row) DROP TABLE fp_reentry_fk2, fp_reentry_pk2; +-- Subtransaction abort during after-trigger firing must not drop FK checks +-- for rows buffered earlier in the same statement. Batching is confined to +-- the top transaction level and the buffered batch is no longer discarded on +-- subxact abort, so the violating rows are detected. +CREATE TABLE fp_subxact_pk (id int PRIMARY KEY); +INSERT INTO fp_subxact_pk SELECT g FROM generate_series(1, 10) g; +CREATE TABLE fp_subxact_fk (a int, tag text); +ALTER TABLE fp_subxact_fk ADD CONSTRAINT fp_subxact_fk_fkey + FOREIGN KEY (a) REFERENCES fp_subxact_pk (id); +CREATE FUNCTION fp_abort_subxact() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.tag = 'boom' THEN + BEGIN PERFORM 1/0; EXCEPTION WHEN division_by_zero THEN NULL; END; + END IF; + RETURN NEW; +END$$; +CREATE TRIGGER fp_subxact_trg AFTER INSERT ON fp_subxact_fk + FOR EACH ROW EXECUTE FUNCTION fp_abort_subxact(); +INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok'); +ERROR: insert or update on table "fp_subxact_fk" violates foreign key constraint "fp_subxact_fk_fkey" +DETAIL: Key (a)=(999) is not present in table "fp_subxact_pk". +DROP TRIGGER fp_subxact_trg ON fp_subxact_fk; +DROP FUNCTION fp_abort_subxact(); +DROP TABLE fp_subxact_fk, fp_subxact_pk; diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 87381194f41..abeb85965b9 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2726,3 +2726,26 @@ BEGIN END$$; SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1 DROP TABLE fp_reentry_fk2, fp_reentry_pk2; + +-- Subtransaction abort during after-trigger firing must not drop FK checks +-- for rows buffered earlier in the same statement. Batching is confined to +-- the top transaction level and the buffered batch is no longer discarded on +-- subxact abort, so the violating rows are detected. +CREATE TABLE fp_subxact_pk (id int PRIMARY KEY); +INSERT INTO fp_subxact_pk SELECT g FROM generate_series(1, 10) g; +CREATE TABLE fp_subxact_fk (a int, tag text); +ALTER TABLE fp_subxact_fk ADD CONSTRAINT fp_subxact_fk_fkey + FOREIGN KEY (a) REFERENCES fp_subxact_pk (id); +CREATE FUNCTION fp_abort_subxact() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.tag = 'boom' THEN + BEGIN PERFORM 1/0; EXCEPTION WHEN division_by_zero THEN NULL; END; + END IF; + RETURN NEW; +END$$; +CREATE TRIGGER fp_subxact_trg AFTER INSERT ON fp_subxact_fk + FOR EACH ROW EXECUTE FUNCTION fp_abort_subxact(); +INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok'); +DROP TRIGGER fp_subxact_trg ON fp_subxact_fk; +DROP FUNCTION fp_abort_subxact(); +DROP TABLE fp_subxact_fk, fp_subxact_pk;