From c2b50a9a16df58540d99fa605e1bbf75502fee7d Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" Date: Thu, 15 May 2025 10:41:18 -0400 Subject: [PATCH 1/3] Fill testing gap for possible referential integrity violation This commit adds a missing isolation test for (non-PERIOD) foreign keys. With REPEATABLE READ, one transaction can insert a referencing row while another deletes the referenced row, and both see a valid state. But after they have committed, the table violates referential integrity. If the INSERT precedes the DELETE, we use a crosscheck snapshot to see the just-added row, so that the DELETE can raise a foreign key error. You can see the table violate referential integrity if you change ri_restrict to pass false for detectNewRows to ri_PerformCheck. A crosscheck snapshot is not needed when the DELETE comes first, because the INSERT's trigger takes a FOR KEY SHARE lock that sees the row now marked for deletion, waits for that transaction to commit, and raises a serialization error. I added a test for that too though. We already have a similar test (in ri-triggers.spec) for SERIALIZABLE snapshot isolation showing that you can implement foreign keys with just pl/pgSQL, but that test does nothing to validate ri_triggers.c. We also have tests (in fk-snapshot.spec) for other concurrency scenarios, but not this one: we test concurrently deleting both the referencing and referenced row, when the constraint activates a cascade/set null action. But those tests don't exercise ri_restrict, and the consequence of omitting a crosscheck comparison is different: a serialization failure, not a referential integrity violation. --- src/test/isolation/expected/fk-snapshot-2.out | 17 ++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/fk-snapshot-2.spec | 33 +++++++++++++++++++ 3 files changed, 51 insertions(+) create mode 100644 src/test/isolation/expected/fk-snapshot-2.out create mode 100644 src/test/isolation/specs/fk-snapshot-2.spec diff --git a/src/test/isolation/expected/fk-snapshot-2.out b/src/test/isolation/expected/fk-snapshot-2.out new file mode 100644 index 000000000000..202d1429a5ab --- /dev/null +++ b/src/test/isolation/expected/fk-snapshot-2.out @@ -0,0 +1,17 @@ +Parsed test spec with 2 sessions + +starting permutation: s2ins s1del s2c s1c +step s2ins: INSERT INTO child VALUES (1, 1); +step s1del: DELETE FROM parent WHERE parent_id = 1; +step s2c: COMMIT; +step s1del: <... completed> +ERROR: update or delete on table "parent" violates foreign key constraint "child_parent_id_fkey" on table "child" +step s1c: COMMIT; + +starting permutation: s1del s2ins s1c s2c +step s1del: DELETE FROM parent WHERE parent_id = 1; +step s2ins: INSERT INTO child VALUES (1, 1); +step s1c: COMMIT; +step s2ins: <... completed> +ERROR: could not serialize access due to concurrent update +step s2c: COMMIT; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index e3c669a29c7a..12b6581d5abe 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -35,6 +35,7 @@ test: fk-deadlock2 test: fk-partitioned-1 test: fk-partitioned-2 test: fk-snapshot +test: fk-snapshot-2 test: subxid-overflow test: eval-plan-qual test: eval-plan-qual-trigger diff --git a/src/test/isolation/specs/fk-snapshot-2.spec b/src/test/isolation/specs/fk-snapshot-2.spec new file mode 100644 index 000000000000..335f763ac3ae --- /dev/null +++ b/src/test/isolation/specs/fk-snapshot-2.spec @@ -0,0 +1,33 @@ +# RI Trigger test +# +# Test C-based referential integrity enforcement. +# Under REPEATABLE READ we need some snapshot trickery in C, +# or we would permit things that violate referential integrity. + +setup +{ + CREATE TABLE parent (parent_id SERIAL NOT NULL PRIMARY KEY); + CREATE TABLE child ( + child_id SERIAL NOT NULL PRIMARY KEY, + parent_id INTEGER REFERENCES parent); + INSERT INTO parent VALUES(1); +} + +teardown { DROP TABLE parent, child; } + +session s1 +setup { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s1del { DELETE FROM parent WHERE parent_id = 1; } +step s1c { COMMIT; } + +session s2 +setup { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s2ins { INSERT INTO child VALUES (1, 1); } +step s2c { COMMIT; } + +# Violates referential integrity unless we use an up-to-date crosscheck snapshot: +permutation s2ins s1del s2c s1c + +# Raises a can't-serialize exception +# when the INSERT trigger does SELECT FOR KEY SHARE: +permutation s1del s2ins s1c s2c From 500e954ecf6ec7f297a7856a8602d25adf78c40a Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" Date: Thu, 15 May 2025 10:43:19 -0400 Subject: [PATCH 2/3] Add test for temporal referential integrity This commit adds an isolation test showing that temporal foreign keys do not permit referential integrity violations under concurrency, like fk-snapshot-2. You can show that the test fails by passing false for detectNewRows in ri_restrict. --- src/test/isolation/expected/fk-snapshot-3.out | 21 ++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/fk-snapshot-3.spec | 40 +++++++++++++++++++ 3 files changed, 62 insertions(+) create mode 100644 src/test/isolation/expected/fk-snapshot-3.out create mode 100644 src/test/isolation/specs/fk-snapshot-3.spec diff --git a/src/test/isolation/expected/fk-snapshot-3.out b/src/test/isolation/expected/fk-snapshot-3.out new file mode 100644 index 000000000000..7d1b9b7eedc4 --- /dev/null +++ b/src/test/isolation/expected/fk-snapshot-3.out @@ -0,0 +1,21 @@ +Parsed test spec with 2 sessions + +starting permutation: s2ins s1del s2c s1c +step s2ins: + INSERT INTO child VALUES ('[1,2)', '[2020-01-01,2030-01-01)', '[1,2)'); + +step s1del: DELETE FROM parent WHERE id = '[1,2)'; +step s2c: COMMIT; +step s1del: <... completed> +ERROR: update or delete on table "parent" violates foreign key constraint "child_parent_id_valid_at_fkey" on table "child" +step s1c: COMMIT; + +starting permutation: s1del s2ins s1c s2c +step s1del: DELETE FROM parent WHERE id = '[1,2)'; +step s2ins: + INSERT INTO child VALUES ('[1,2)', '[2020-01-01,2030-01-01)', '[1,2)'); + +step s1c: COMMIT; +step s2ins: <... completed> +ERROR: could not serialize access due to concurrent update +step s2c: COMMIT; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 12b6581d5abe..bd903bee8232 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -36,6 +36,7 @@ test: fk-partitioned-1 test: fk-partitioned-2 test: fk-snapshot test: fk-snapshot-2 +test: fk-snapshot-3 test: subxid-overflow test: eval-plan-qual test: eval-plan-qual-trigger diff --git a/src/test/isolation/specs/fk-snapshot-3.spec b/src/test/isolation/specs/fk-snapshot-3.spec new file mode 100644 index 000000000000..6ae32ecf2fc6 --- /dev/null +++ b/src/test/isolation/specs/fk-snapshot-3.spec @@ -0,0 +1,40 @@ +# RI Trigger test +# +# Test C-based temporal referential integrity enforcement. +# Under REPEATABLE READ we need some snapshot trickery in C, +# or we would permit things that violate referential integrity. + +setup +{ + CREATE TABLE parent ( + id int4range NOT NULL, + valid_at daterange NOT NULL, + PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)); + CREATE TABLE child ( + id int4range NOT NULL, + valid_at daterange NOT NULL, + parent_id int4range, + FOREIGN KEY (parent_id, PERIOD valid_at) REFERENCES parent); + INSERT INTO parent VALUES ('[1,2)', '[2020-01-01,2030-01-01)'); +} + +teardown { DROP TABLE parent, child; } + +session s1 +setup { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s1del { DELETE FROM parent WHERE id = '[1,2)'; } +step s1c { COMMIT; } + +session s2 +setup { BEGIN ISOLATION LEVEL REPEATABLE READ; } +step s2ins { + INSERT INTO child VALUES ('[1,2)', '[2020-01-01,2030-01-01)', '[1,2)'); +} +step s2c { COMMIT; } + +# Violates referential integrity unless we use an up-to-date crosscheck snapshot: +permutation s2ins s1del s2c s1c + +# Raises a can't-serialize exception +# when the INSERT trigger does SELECT FOR KEY SHARE: +permutation s1del s2ins s1c s2c From f65d89d12f6c8d71ef8dd8abf3f049cd02d5c752 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" Date: Thu, 15 May 2025 10:33:04 -0400 Subject: [PATCH 3/3] Improve comment about snapshot macros The comment mistakenly had "the others" for "the other", but this commit also reorders the comment so it matches the macros below. Now we describe the levels in increasing strictness. Finally, it seems easier to follow if we introduce one level at a time, rather than describing two, followed by "the other" (and then jumping back to one of the first two). --- src/include/access/xact.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/include/access/xact.h b/src/include/access/xact.h index 7f11b9197994..531dbc4a16f4 100644 --- a/src/include/access/xact.h +++ b/src/include/access/xact.h @@ -43,8 +43,8 @@ extern PGDLLIMPORT int XactIsoLevel; /* * We implement three isolation levels internally. - * The two stronger ones use one snapshot per database transaction; - * the others use one snapshot per statement. + * The weakest uses one snapshot per statement; + * the two stronger levels use one snapshot per database transaction. * Serializable uses predicate locks in addition to snapshots. * These macros should be used to check which isolation level is selected. */