diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 3a89149016f..9e2d507c8a9 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -5245,6 +5245,7 @@ CommitSubTransaction(void) s->parent->subTransactionId); AtEOSubXact_HashTables(true, s->nestingLevel); AtEOSubXact_PgStat(true, s->nestingLevel); + AtEOSubXact_RI(true, s->subTransactionId, s->parent->subTransactionId); AtSubCommit_Snapshot(s->nestingLevel); /* @@ -5419,6 +5420,7 @@ AbortSubTransaction(void) s->parent->subTransactionId); AtEOSubXact_HashTables(false, s->nestingLevel); AtEOSubXact_PgStat(false, s->nestingLevel); + AtEOSubXact_RI(false, s->subTransactionId, s->parent->subTransactionId); AtSubAbort_Snapshot(s->nestingLevel); } diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 5a209673b61..439376a6cc2 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -287,6 +287,14 @@ typedef struct RI_FastPathEntry * re-entrant ri_FastPathBatchAdd from user code run during the flush. */ bool flushing; + + /* + * Subtransaction whose resource owner opened this entry's relations. + * AtEOSubXact_RI() drops only entries matching an aborting subxact, so a + * subxact abort during outer-level trigger firing leaves the outer batch + * intact. + */ + SubTransactionId subid; } RI_FastPathEntry; /* @@ -512,9 +520,7 @@ RI_FKey_check(TriggerData *trigdata) */ if (ri_fastpath_is_applicable(riinfo)) { - if (AfterTriggerIsActive() && - GetCurrentTransactionNestLevel() == 1 && - !ri_fastpath_flushing) + if (AfterTriggerIsActive() && !ri_fastpath_flushing) { /* Batched path: buffer and probe in groups */ ri_FastPathBatchAdd(riinfo, fk_rel, newslot); @@ -522,15 +528,11 @@ RI_FKey_check(TriggerData *trigdata) else { /* - * Per-row path, used when batching is not safe or not applicable: + * Per-row path, used when batching is 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 @@ -2969,6 +2971,14 @@ ri_FastPathBatchAdd(RI_ConstraintInfo *riinfo, return; } + /* + * A batch is filled and flushed within a single trigger-firing cycle, so + * every row added to an entry comes from the subtransaction that created + * it. AtEOSubXact_RI() relies on this to identify an aborting + * subtransaction's entries by the subid stamped at entry creation. + */ + Assert(fpentry->subid == GetCurrentSubTransactionId()); + /* * Buffer the row. A full batch is flushed below and re-entry is handled * above, so there is always room here; the bounds check just guards the @@ -4480,6 +4490,73 @@ AtEOXact_RI(bool isCommit) } } +/* + * AtEOSubXact_RI + * Reset fast-path batching state at subtransaction end. + * + * Called from CommitSubTransaction() with isCommit true and from + * AbortSubTransaction() with isCommit false, in both cases after the + * subtransaction's ResourceOwnerRelease(). + * + * Fast-path cache entries are normally flushed and removed at the end of + * their trigger-firing cycle, and the cache is destroyed when its last entry + * is removed. Thus, at a normal subtransaction boundary this is a no-op. + * + * The exception is a batch flush that errors out partway and is caught by this + * subtransaction (e.g. a PL/pgSQL EXCEPTION block): ri_FastPathEndBatch()'s + * teardown was skipped, so the cache still contains entries whose relations + * were opened under this subtransaction's resource owner. That owner has + * just released those relations, making the entries stale. Remove those + * entries so a later firing cycle cannot reuse them. Entries belonging to + * outer subtransactions remain valid and are preserved. + * + * The remaining slot storage and per-entry flush contexts are reclaimed when + * TopTransactionContext is reset at top-level transaction end. + */ +void +AtEOSubXact_RI(bool isCommit, SubTransactionId mySubid, + SubTransactionId parentSubid) +{ + HASH_SEQ_STATUS status; + RI_FastPathEntry *entry; + long remaining; + + if (ri_fastpath_cache == NULL) + return; + + /* Process only entries belonging to the ending subtransaction. */ + hash_seq_init(&status, ri_fastpath_cache); + while ((entry = hash_seq_search(&status)) != NULL) + { + if (entry->subid != mySubid) + continue; + + if (isCommit) + { + /* + * A committing subxact's entry should already have been flushed + * and torn down at its statement's end (ri_FastPathEndBatch()), + * so we don't expect to find one here. If we do, reassign it to + * the parent so it's still cleaned up rather than left under a + * subxact id that no longer exists. + */ + Assert(false); + entry->subid = parentSubid; + } + else + hash_search(ri_fastpath_cache, &entry->key, HASH_REMOVE, NULL); + } + + /* If that emptied the cache, drop it so the next batch starts clean. */ + remaining = hash_get_num_entries(ri_fastpath_cache); + if (remaining == 0) + { + hash_destroy(ri_fastpath_cache); + ri_fastpath_cache = NULL; + ri_fastpath_flushing = false; + } +} + /* * ri_FastPathGetEntry * Look up or create a per-batch cache entry for the given constraint. @@ -4605,6 +4682,7 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) entry->flushing = false; entry->batch_count = 0; + entry->subid = GetCurrentSubTransactionId(); } return entry; diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 1f268f87957..fecdb785f35 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -312,5 +312,7 @@ extern bool AfterTriggerIsActive(void); extern int AfterTriggerCurrentQueryDepth(void); extern void AtEOXact_RI(bool isCommit); +extern void AtEOSubXact_RI(bool isCommit, SubTransactionId mySubid, + SubTransactionId parentSubid); #endif /* TRIGGER_H */ diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 126c251bbda..ac044eb40fa 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -3951,6 +3951,76 @@ ROLLBACK; RESET ROLE; DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk; DROP ROLE regress_fpav_role; +-- Re-entrant fast-path check inside a committing subtransaction. An AFTER +-- trigger on one FK table runs FK DML on a second FK table inside a PL/pgSQL +-- BEGIN ... EXCEPTION block, so the inner check batches in its own +-- trigger-firing cycle nested in the outer check's. The inner cycle must +-- register its own end-of-batch callback and flush -- otherwise its FK check +-- is skipped (an orphan commits) and its relations leak. +CREATE TABLE fp_inner_pk (id int PRIMARY KEY); +INSERT INTO fp_inner_pk VALUES (1); +CREATE TABLE fp_inner_fk (a int REFERENCES fp_inner_pk (id)); +CREATE TABLE fp_outer_pk (id int PRIMARY KEY); +INSERT INTO fp_outer_pk SELECT g FROM generate_series(1, 64) g; +CREATE FUNCTION fp_reentry_subxact() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.a = 32 THEN + BEGIN + INSERT INTO fp_inner_fk VALUES (999); -- violates; must be caught + EXCEPTION WHEN foreign_key_violation THEN + NULL; + END; + END IF; + RETURN NEW; +END$$; +CREATE TABLE fp_outer_fk (a int REFERENCES fp_outer_pk (id)); +CREATE TRIGGER fp_reentry_subxact_trg AFTER INSERT ON fp_outer_fk + FOR EACH ROW EXECUTE FUNCTION fp_reentry_subxact(); +INSERT INTO fp_outer_fk SELECT g FROM generate_series(1, 64) g; +SELECT count(*) AS outer_rows FROM fp_outer_fk; -- 64, outer batch intact + outer_rows +------------ + 64 +(1 row) + +SELECT count(*) AS inner_rows FROM fp_inner_fk; -- 0, inner check caught + inner_rows +------------ + 0 +(1 row) + +DROP TRIGGER fp_reentry_subxact_trg ON fp_outer_fk; +DROP FUNCTION fp_reentry_subxact(); +DROP TABLE fp_outer_fk, fp_outer_pk, fp_inner_fk, fp_inner_pk; +-- A nested trigger-firing cycle that checks the same constraint must use a +-- separate cache entry. The inner violation is caught by its subtransaction, +-- while the valid outer row remains buffered and is checked normally. +CREATE TABLE fp_same_pk (id int PRIMARY KEY); +INSERT INTO fp_same_pk VALUES (1); +CREATE TABLE fp_same_fk (a int REFERENCES fp_same_pk (id)); +CREATE FUNCTION fp_reentry_same_constraint() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.a = 1 THEN + BEGIN + INSERT INTO fp_same_fk VALUES (999); + EXCEPTION WHEN foreign_key_violation THEN + NULL; + END; + END IF; + RETURN NEW; +END$$; +CREATE TRIGGER fp_reentry_same_constraint_trg AFTER INSERT ON fp_same_fk + FOR EACH ROW EXECUTE FUNCTION fp_reentry_same_constraint(); +INSERT INTO fp_same_fk VALUES (1); +SELECT * FROM fp_same_fk; + a +--- + 1 +(1 row) + +DROP TRIGGER fp_reentry_same_constraint_trg ON fp_same_fk; +DROP FUNCTION fp_reentry_same_constraint(); +DROP TABLE fp_same_fk, fp_same_pk; -- 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 diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 7319de6a280..a93e81b42bc 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2898,6 +2898,63 @@ RESET ROLE; DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk; DROP ROLE regress_fpav_role; +-- Re-entrant fast-path check inside a committing subtransaction. An AFTER +-- trigger on one FK table runs FK DML on a second FK table inside a PL/pgSQL +-- BEGIN ... EXCEPTION block, so the inner check batches in its own +-- trigger-firing cycle nested in the outer check's. The inner cycle must +-- register its own end-of-batch callback and flush -- otherwise its FK check +-- is skipped (an orphan commits) and its relations leak. +CREATE TABLE fp_inner_pk (id int PRIMARY KEY); +INSERT INTO fp_inner_pk VALUES (1); +CREATE TABLE fp_inner_fk (a int REFERENCES fp_inner_pk (id)); +CREATE TABLE fp_outer_pk (id int PRIMARY KEY); +INSERT INTO fp_outer_pk SELECT g FROM generate_series(1, 64) g; +CREATE FUNCTION fp_reentry_subxact() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.a = 32 THEN + BEGIN + INSERT INTO fp_inner_fk VALUES (999); -- violates; must be caught + EXCEPTION WHEN foreign_key_violation THEN + NULL; + END; + END IF; + RETURN NEW; +END$$; +CREATE TABLE fp_outer_fk (a int REFERENCES fp_outer_pk (id)); +CREATE TRIGGER fp_reentry_subxact_trg AFTER INSERT ON fp_outer_fk + FOR EACH ROW EXECUTE FUNCTION fp_reentry_subxact(); +INSERT INTO fp_outer_fk SELECT g FROM generate_series(1, 64) g; +SELECT count(*) AS outer_rows FROM fp_outer_fk; -- 64, outer batch intact +SELECT count(*) AS inner_rows FROM fp_inner_fk; -- 0, inner check caught +DROP TRIGGER fp_reentry_subxact_trg ON fp_outer_fk; +DROP FUNCTION fp_reentry_subxact(); +DROP TABLE fp_outer_fk, fp_outer_pk, fp_inner_fk, fp_inner_pk; + +-- A nested trigger-firing cycle that checks the same constraint must use a +-- separate cache entry. The inner violation is caught by its subtransaction, +-- while the valid outer row remains buffered and is checked normally. +CREATE TABLE fp_same_pk (id int PRIMARY KEY); +INSERT INTO fp_same_pk VALUES (1); +CREATE TABLE fp_same_fk (a int REFERENCES fp_same_pk (id)); +CREATE FUNCTION fp_reentry_same_constraint() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.a = 1 THEN + BEGIN + INSERT INTO fp_same_fk VALUES (999); + EXCEPTION WHEN foreign_key_violation THEN + NULL; + END; + END IF; + RETURN NEW; +END$$; +CREATE TRIGGER fp_reentry_same_constraint_trg AFTER INSERT ON fp_same_fk + FOR EACH ROW EXECUTE FUNCTION fp_reentry_same_constraint(); +INSERT INTO fp_same_fk VALUES (1); +SELECT * FROM fp_same_fk; +DROP TRIGGER fp_reentry_same_constraint_trg ON fp_same_fk; +DROP FUNCTION fp_reentry_same_constraint(); +DROP TABLE fp_same_fk, fp_same_pk; + -- 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