Обсуждение: Foreign key isolation tests
Here are a couple new isolation tests for foreign keys, based on
feedback from the Advanced Patch Review session at PGConf.dev.
The goal is to show that temporal foreign keys do not violate
referential integrity under concurrency. Non-temporal foreign keys use
a crosscheck snapshot to avoid this. Temporal foreign keys do the
same, but this test gives us more assurance.
I noticed that traditional foreign keys didn't have a test either, at
least for the case we discussed: one transaction INSERTS a referencing
row, while the other DELETEs the referenced row. So I have two patches
here: one adding tests for the traditional case; another, the temporal
case.
There is a final patch improving the comment for some snapshot macros.
While fixing a typo I thought I could improve their clarity a bit as
well.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Вложения
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
Hi Paul,
Thanks for this patch!
Lilian and I have reviewed your patches.
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?
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?
Also in v1-0002 you only test 'REPEATABLE READ' isolation level, what about the
other two? ('READ COMMITTED', 'SERIALIZABLE')
Another question we wanted to clarify, what would be the right bash script
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
Kindest regards.
[1] https://drive.google.com/drive/folders/17Yc8C9p0foZSnT-PtIXgfiFlAY80V4Mc
The new status of this patch is: Waiting on Author
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 ~{:-)
pj@illuminatedcomputing.com
Вложения
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed Hi Paul, Thank you for your prompt reply. > I have the same --enable-* options. > But does TESTNAME actually restrict your run to just those three? > It doesn't for me. It does not for us either. We somehow overlooked, sorry. It will run all the 118 tests specified at isolation_schedule file. > As far as I know there is no way to run selected isolation tests, > other than editing the isolation_schedule file Thank you for explaining this, indeed modifying isolation_schedule file does the trick on testing single spec only. > Is there documentation saying that it should do something? No there is none. TESTNAME variable was suggested by Claude 4.0 Sonnet. We have rerun again 'make world' and 'make check-world' with your current changes, all tests are passing. Thank you for all your clarifications! We believe these are ready for committer. Kindest regards. The new status of this patch is: Ready for Committer
On Fri, 11 Jul 2025 at 23:22, Paul Jungwirth <pj@illuminatedcomputing.com> wrote: > > 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. > I agree that additional testing in this area is valuable. Patch 0001 looks reasonable to me, on a quick read-through. In patch 0002, I think it would be valuable to also test updating the parent row to time periods consistent and not consistent with the insert, to confirm that that behaves correctly. Regards, Dean
On Fri, Jul 18, 2025 at 3:29 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> I agree that additional testing in this area is valuable. Patch 0001
> looks reasonable to me, on a quick read-through. In patch 0002, I
> think it would be valuable to also test updating the parent row to
> time periods consistent and not consistent with the insert, to confirm
> that that behaves correctly.
Thanks for taking a look! Here are new patches with those extra tests.
There are extensive regress tests already, so I just tested the same
concurrency pattern. I think the results are okay. I do get a
can't-serialize exception for a couple valid changes under REPEATABLE
READ and SERIALIZE, but I think they are expected and not a bug. (I
think you would see the same thing outside of FKs.)
--
Paul ~{:-)
pj@illuminatedcomputing.com
Вложения
On Fri, Jul 18, 2025 at 7:26 PM Paul A Jungwirth <pj@illuminatedcomputing.com> wrote: > Thanks for taking a look! Here are new patches with those extra tests. > There are extensive regress tests already, so I just tested the same > concurrency pattern. I think the results are okay. I do get a > can't-serialize exception for a couple valid changes under REPEATABLE > READ and SERIALIZE, but I think they are expected and not a bug. (I > think you would see the same thing outside of FKs.) 0001 and 0003 look OK to me on a quick read-through. 0002 seems to do something horrible to isolation_schedule. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, Aug 11, 2025 at 8:03 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Jul 18, 2025 at 7:26 PM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> > Thanks for taking a look! Here are new patches with those extra tests.
> > There are extensive regress tests already, so I just tested the same
> > concurrency pattern. I think the results are okay. I do get a
> > can't-serialize exception for a couple valid changes under REPEATABLE
> > READ and SERIALIZE, but I think they are expected and not a bug. (I
> > think you would see the same thing outside of FKs.)
>
> 0001 and 0003 look OK to me on a quick read-through. 0002 seems to do
> something horrible to isolation_schedule.
Ugh, sorry about that. Here is a fix.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Вложения
Thank you, I pushed 0001 and 0002. Regarding 0003, On 2025-Aug-11, Paul A Jungwirth wrote: > /* > * 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. > */ The wording of the last sentence (which you don't change) is a bit funny, because the macros aren't really to be used to check which isolation level is selected (which an interested observer could determine simply by looking at XactIsoLevel). What they do is implement a layer on top of the selected isolation level -- they are there to know which implementation to use depending on the isolation level. I also think that, for the explanation about serializable, we should change "in addition to snapshots" to "in addition to the snapshot", calling out the fact that the transaction will in fact use a single snapshot throughout. So how about something like this? (I include the macros in question so that we see exactly what we're talking about). /* * We implement three isolation levels internally. * The weakest uses one snapshot per statement; * the two stronger levels use one snapshot per database transaction. * Serializable uses predicate locks in addition to the snapshot. * These macros can be used to determine which implementation to use * depending on the prevailing serialization level. */ #define IsolationUsesXactSnapshot() (XactIsoLevel >= XACT_REPEATABLE_READ) #define IsolationIsSerializable() (XactIsoLevel == XACT_SERIALIZABLE) -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Porque Kim no hacía nada, pero, eso sí, con extraordinario éxito" ("Kim", Kipling)
On Thu, Sep 11, 2025 at 9:30 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> The wording of the last sentence (which you don't change) is a bit
> funny, because the macros aren't really to be used to check which
> isolation level is selected (which an interested observer could
> determine simply by looking at XactIsoLevel). What they do is implement
> a layer on top of the selected isolation level -- they are there to know
> which implementation to use depending on the isolation level.
>
> I also think that, for the explanation about serializable, we should
> change "in addition to snapshots" to "in addition to the snapshot",
> calling out the fact that the transaction will in fact use a single
> snapshot throughout.
>
> So how about something like this? (I include the macros in question so
> that we see exactly what we're talking about).
>
> /*
> * We implement three isolation levels internally.
> * The weakest uses one snapshot per statement;
> * the two stronger levels use one snapshot per database transaction.
> * Serializable uses predicate locks in addition to the snapshot.
> * These macros can be used to determine which implementation to use
> * depending on the prevailing serialization level.
> */
> #define IsolationUsesXactSnapshot() (XactIsoLevel >= XACT_REPEATABLE_READ)
> #define IsolationIsSerializable() (XactIsoLevel == XACT_SERIALIZABLE)
These all seem like improvements to me. Thanks for the review!
--
Paul ~{:-)
pj@illuminatedcomputing.com
On 2025-Sep-11, Paul A Jungwirth wrote: > These all seem like improvements to me. Thanks for the review! Great, pushed that way, thanks. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/