On 7/11/25 11:50, Rustam ALLAKOV wrote:
Hi Paul,
Thanks for this patch!
Lilian and I have reviewed your patches.
Thank you for the review! I've attached new patches addressing your feedback.
All patches apply and all tests are passing.
Since you are adding tests, we expected to see some improvement on the test
coverage. Yet no significant improvement was observed. We have tested vanila
postgres at commit 931766aaec58b2ce09c82203456877e0b05e1271, run coverage.
Then we have applied first patch v1-0001, tested and run coverage.
Lastly v2-0001 was applied on top of v1-0001, tested and run coverage.
All three coverage reports can be found here [1].
We expected some increase in coverage especially in this file
`src/backend/utils/adt/ri_triggers.c` perhaps we are looking at the wrong
file?
A system isn't fully tested just because all lines are tested.
For instance suppose you have `int add(int x, int y) { return x + y; }`.
A test may confirm that `add(1, 1)` equals 2, and you have 100% test coverage,
but `add(INT_MAX, 1)` equaling -2147483648 is probably still a bug.
Similarly, these tests don't cover untested *lines*, but untested *scenarios*.
Since this is a tricky scenario to get correct, I think we should have a test
for it.
I was asked to add these tests for temporal foreign keys, and I was surprised
to find
we don't have them for regular foreign keys either, so these patches add both.
If you compile with this change, all old tests still pass, but the new tests
will fail:
diff --git a/src/backend/utils/adt/ri_triggers.c
b/src/backend/utils/adt/ri_triggers.c
index 059fc5ebf60..8738b2d7e03 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -892,7 +892,7 @@ ri_restrict(TriggerData *trigdata, bool
is_no_action)
fk_rel, pk_rel,
oldslot, NULL,
!is_no_action,
- true, /* must detect new rows */
+ false, /* must detect new rows */
SPI_OK_SELECT);
if (SPI_finish() != SPI_OK_FINISH)
In addition to that, related to the comment
'Violates referential integrity unless we use an up-to-date crosscheck
snapshot:'
Could you please clarify:
Why is this necessary to specify usage of an up-to-date crosscheck snapshot?
When would the crosscheck snapshot be not up to date?
Should we add tests for up-to-date crosscheck snapshots?
The point is that the crosscheck snapshot *is* up to date, compared to the
current snapshot.
The adjective is descriptive, not limiting.
I re-worded the comment a bit to hopefully make this more clear.
I think that answers all the questions you're asking here, but to give some
more context:
The crosscheck snapshot is required to prevent bad rows sneaking past the
foreign key constraint.
Take the two tables from fk-snapshot-2.spec. The parent has a row with
parent_id 1.
Now imagine you have two connections, both REPEATABLE READ, that do things in
this order
(without a crosscheck snapshot):
connection 2: insert into child (child_id, parent_id) values (1, 1);
connection 2: -- RI triggers run, the constraint looks good.
connection 2: -- We have a FOR KEY SHARE lock on the parent row.
connection 1: delete from parent where parent_id = 1;
connection 1: -- We can't delete the row until we get our own lock,
connection 1: -- which conflicts with FOR KEY SHARE.
connection 1: -- (Since this is a tuple lock, in pg_locks you see us waiting on
connection 1: -- the transactionid of connection 2.)
connection 2: commit;
connection 1: -- We get the lock, delete the row, and run our RI check.
connection 1: -- We don't see the new insert, so the constraint looks good.
connection 1: commit;
We have violated the foreign key!
Also in v1-0002 you only test 'REPEATABLE READ' isolation level, what about the
other two? ('READ COMMITTED', 'SERIALIZABLE')
In READ COMMITTED, connection 1 is allowed to see the inserted row, so it
correctly rejects the delete anyway.
In SERIALIZABLE, the sequence would raise a cannot-serialize exception anyway,
although the cross-check
snapshot helps us give a better error message.
I added these cases to fk-snapshot-{2,3}.spec. I don't think they are as
important as REPEATABLE READ,
but for something as fundamental as foreign keys it is reassuring to have them,
and I like that they document what happens.
to test this specific feature only instead of running the whole test suit.
These are the steps we came up with so far: (we use Macos)
./configure --prefix=$PGINSTALL/paul --enable-cassert --enable-debug \
--enable-coverage --enable-tap-tests --without-icu --without-zstd \
--without-zlib CFLAGS="-ggdb -O0 -fno-omit-frame-pointer" CPPFLAGS="-g -O0"
make -j12 -s
make -j12 install
cd src/test/isolation
make check TESTNAME="fk-snapshot fk-snapshot-2 fk-snapshot-3"
make coverage-html
I have the same --enable-* options.
But does TESTNAME actually restrict your run to just those three? It doesn't
for me.
As far as I know there is no way to run selected isolation tests, other than
editing the isolation_schedule file.
For regress tests you can say `make check-tests TESTS=bit`. (Note the different
Makefile target.)
Within src/test/subscription you can use `make check
PROVE_TESTS=t/034_temporal.pl`.
Weirdly `grep -r TESTNAME src/test/isolation` does say that
pg_isolation_regress matches.
Is there documentation saying that it should do something?
If there is a way to run just one isolation test, I'd love to know.
Rebased to 4df477153a.
Yours,
--
Paul ~{:-)
p...@illuminatedcomputing.com
From 3df781e45ae48767620ed361525557f9ee60b90b Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <p...@illuminatedcomputing.com>
Date: Thu, 15 May 2025 10:41:18 -0400
Subject: [PATCH v2 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 | 61 +++++++++++++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/fk-snapshot-2.spec | 50 +++++++++++++++
3 files changed, 112 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 00000000000..0a4c9646fca
--- /dev/null
+++ b/src/test/isolation/expected/fk-snapshot-2.out
@@ -0,0 +1,61 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1rr s2rr s2ins s1del s2c s1c
+step s1rr: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2rr: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2ins: INSERT INTO child VALUES (1, 1);
+step s1del: DELETE FROM parent WHERE parent_id = 1; <waiting ...>
+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: s1rr s2rr s1del s2ins s1c s2c
+step s1rr: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2rr: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s1del: DELETE FROM parent WHERE parent_id = 1;
+step s2ins: INSERT INTO child VALUES (1, 1); <waiting ...>
+step s1c: COMMIT;
+step s2ins: <... completed>
+ERROR: could not serialize access due to concurrent update
+step s2c: COMMIT;
+
+starting permutation: s1rc s2rc s2ins s1del s2c s1c
+step s1rc: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s2rc: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s2ins: INSERT INTO child VALUES (1, 1);
+step s1del: DELETE FROM parent WHERE parent_id = 1; <waiting ...>
+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: s1rc s2rc s1del s2ins s1c s2c
+step s1rc: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s2rc: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s1del: DELETE FROM parent WHERE parent_id = 1;
+step s2ins: INSERT INTO child VALUES (1, 1); <waiting ...>
+step s1c: COMMIT;
+step s2ins: <... completed>
+ERROR: insert or update on table "child" violates foreign key constraint "child_parent_id_fkey"
+step s2c: COMMIT;
+
+starting permutation: s1ser s2ser s2ins s1del s2c s1c
+step s1ser: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s2ser: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s2ins: INSERT INTO child VALUES (1, 1);
+step s1del: DELETE FROM parent WHERE parent_id = 1; <waiting ...>
+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: s1ser s2ser s1del s2ins s1c s2c
+step s1ser: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s2ser: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s1del: DELETE FROM parent WHERE parent_id = 1;
+step s2ins: INSERT INTO child VALUES (1, 1); <waiting ...>
+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 e3c669a29c7..12b6581d5ab 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 00000000000..94cd151aab9
--- /dev/null
+++ b/src/test/isolation/specs/fk-snapshot-2.spec
@@ -0,0 +1,50 @@
+# 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
+step s1rc { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step s1rr { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step s1ser { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step s1del { DELETE FROM parent WHERE parent_id = 1; }
+step s1c { COMMIT; }
+
+session s2
+step s2rc { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step s2rr { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step s2ser { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step s2ins { INSERT INTO child VALUES (1, 1); }
+step s2c { COMMIT; }
+
+# Violates referential integrity unless we use a crosscheck snapshot,
+# which is up-to-date compared with the transaction's snapshot.
+permutation s1rr s2rr s2ins s1del s2c s1c
+
+# Raises a can't-serialize exception
+# when the INSERT trigger does SELECT FOR KEY SHARE:
+permutation s1rr s2rr s1del s2ins s1c s2c
+
+# Test the same scenarios in READ COMMITTED:
+# A crosscheck snapshot is not required here.
+permutation s1rc s2rc s2ins s1del s2c s1c
+permutation s1rc s2rc s1del s2ins s1c s2c
+
+# Test the same scenarios in SERIALIZABLE:
+# We should report the FK violation:
+permutation s1ser s2ser s2ins s1del s2c s1c
+# We raise a concurrent update error
+# which is good enough:
+permutation s1ser s2ser s1del s2ins s1c s2c
--
2.39.5
From 5563a923d8b3bbdb0f7eaab00d4994633fc17677 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <p...@illuminatedcomputing.com>
Date: Thu, 15 May 2025 10:43:19 -0400
Subject: [PATCH v2 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 | 73 +++++++++++++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/fk-snapshot-3.spec | 56 ++++++++++++++
3 files changed, 130 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 00000000000..425c76df3b4
--- /dev/null
+++ b/src/test/isolation/expected/fk-snapshot-3.out
@@ -0,0 +1,73 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1rr s2rr s2ins s1del s2c s1c
+step s1rr: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2rr: BEGIN ISOLATION LEVEL REPEATABLE READ;
+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)'; <waiting ...>
+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: s1rr s2rr s1del s2ins s1c s2c
+step s1rr: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2rr: BEGIN ISOLATION LEVEL REPEATABLE READ;
+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)');
+ <waiting ...>
+step s1c: COMMIT;
+step s2ins: <... completed>
+ERROR: could not serialize access due to concurrent update
+step s2c: COMMIT;
+
+starting permutation: s1rc s2rc s2ins s1del s2c s1c
+step s1rc: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s2rc: BEGIN ISOLATION LEVEL READ COMMITTED;
+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)'; <waiting ...>
+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: s1rc s2rc s1del s2ins s1c s2c
+step s1rc: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s2rc: BEGIN ISOLATION LEVEL READ COMMITTED;
+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)');
+ <waiting ...>
+step s1c: COMMIT;
+step s2ins: <... completed>
+ERROR: insert or update on table "child" violates foreign key constraint "child_parent_id_valid_at_fkey"
+step s2c: COMMIT;
+
+starting permutation: s1ser s2ser s2ins s1del s2c s1c
+step s1ser: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s2ser: BEGIN ISOLATION LEVEL SERIALIZABLE;
+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)'; <waiting ...>
+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: s1ser s2ser s1del s2ins s1c s2c
+step s1ser: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s2ser: BEGIN ISOLATION LEVEL SERIALIZABLE;
+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)');
+ <waiting ...>
+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 12b6581d5ab..bd903bee823 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 00000000000..71aad45fae8
--- /dev/null
+++ b/src/test/isolation/specs/fk-snapshot-3.spec
@@ -0,0 +1,56 @@
+# 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
+step s1rc { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step s1rr { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step s1ser { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step s1del { DELETE FROM parent WHERE id = '[1,2)'; }
+step s1c { COMMIT; }
+
+session s2
+step s2rc { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step s2rr { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step s2ser { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+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 s1rr s2rr s2ins s1del s2c s1c
+
+# Raises a can't-serialize exception
+# when the INSERT trigger does SELECT FOR KEY SHARE:
+permutation s1rr s2rr s1del s2ins s1c s2c
+
+# Test the same scenarios in READ COMMITTED:
+# A crosscheck snapshot is not required here.
+permutation s1rc s2rc s2ins s1del s2c s1c
+permutation s1rc s2rc s1del s2ins s1c s2c
+
+# Test the same scenarios in SERIALIZABLE:
+# We should report the FK violation:
+permutation s1ser s2ser s2ins s1del s2c s1c
+# We raise a concurrent update error
+# which is good enough:
+permutation s1ser s2ser s1del s2ins s1c s2c
--
2.39.5
From 3b7313fe0d80bac66adb606fed07d9242712f35c Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <p...@illuminatedcomputing.com>
Date: Thu, 15 May 2025 10:33:04 -0400
Subject: [PATCH v2 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 b2bc10ee041..fdcff3411ad 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.
*/
--
2.39.5
