From 60f227316c0ebf5f4f8296f11cedc9360e9cb8ae Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Tue, 28 Nov 2023 11:59:09 +0200 Subject: [PATCH] Fix assertions with RI triggers in heap_update and heap_delete. If the tuple being updated is not visible to the crosscheck snapshot, we return TM_Updated but the assertions would not hold in that case. Move them to before the cross-check. Fixes bug #17893. Backpatch to all supported versions. Author: Alexander Lakhin Backpatch-through: 12 Discussion: https://www.postgresql.org/message-id/17893-35847009eec517b5%40postgresql.org --- src/backend/access/heap/heapam.c | 41 ++++++++++++--------- src/include/access/tableam.h | 4 +- src/test/isolation/expected/fk-snapshot.out | 22 +++++++++++ src/test/isolation/specs/fk-snapshot.spec | 17 ++++++++- 4 files changed, 64 insertions(+), 20 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 14de8158d4..f938715359 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -2708,13 +2708,7 @@ l1: result = TM_Deleted; } - if (crosscheck != InvalidSnapshot && result == TM_Ok) - { - /* Perform additional check for transaction-snapshot mode RI updates */ - if (!HeapTupleSatisfiesVisibility(&tp, crosscheck, buffer)) - result = TM_Updated; - } - + /* sanity check the result HeapTupleSatisfiesUpdate() and the logic above */ if (result != TM_Ok) { Assert(result == TM_SelfModified || @@ -2724,6 +2718,17 @@ l1: Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID)); Assert(result != TM_Updated || !ItemPointerEquals(&tp.t_self, &tp.t_data->t_ctid)); + } + + if (crosscheck != InvalidSnapshot && result == TM_Ok) + { + /* Perform additional check for transaction-snapshot mode RI updates */ + if (!HeapTupleSatisfiesVisibility(&tp, crosscheck, buffer)) + result = TM_Updated; + } + + if (result != TM_Ok) + { tmfd->ctid = tp.t_data->t_ctid; tmfd->xmax = HeapTupleHeaderGetUpdateXid(tp.t_data); if (result == TM_SelfModified) @@ -3342,16 +3347,7 @@ l2: result = TM_Deleted; } - if (crosscheck != InvalidSnapshot && result == TM_Ok) - { - /* Perform additional check for transaction-snapshot mode RI updates */ - if (!HeapTupleSatisfiesVisibility(&oldtup, crosscheck, buffer)) - { - result = TM_Updated; - Assert(!ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid)); - } - } - + /* Sanity check the result HeapTupleSatisfiesUpdate() and the logic above */ if (result != TM_Ok) { Assert(result == TM_SelfModified || @@ -3361,6 +3357,17 @@ l2: Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID)); Assert(result != TM_Updated || !ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid)); + } + + if (crosscheck != InvalidSnapshot && result == TM_Ok) + { + /* Perform additional check for transaction-snapshot mode RI updates */ + if (!HeapTupleSatisfiesVisibility(&oldtup, crosscheck, buffer)) + result = TM_Updated; + } + + if (result != TM_Ok) + { tmfd->ctid = oldtup.t_data->t_ctid; tmfd->xmax = HeapTupleHeaderGetUpdateXid(oldtup.t_data); if (result == TM_SelfModified) diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index 9ab7201f4d..87227d8bd2 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -1479,8 +1479,8 @@ table_multi_insert(Relation rel, TupleTableSlot **slots, int nslots, * TM_BeingModified (the last only possible if wait == false). * * In the failure cases, the routine fills *tmfd with the tuple's t_ctid, - * t_xmax, and, if possible, and, if possible, t_cmax. See comments for - * struct TM_FailureData for additional info. + * t_xmax, and, if possible, t_cmax. See comments for struct + * TM_FailureData for additional info. */ static inline TM_Result table_tuple_delete(Relation rel, ItemPointer tid, CommandId cid, diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out index 5faf80d6ce..bdd26bac6c 100644 --- a/src/test/isolation/expected/fk-snapshot.out +++ b/src/test/isolation/expected/fk-snapshot.out @@ -122,3 +122,25 @@ a 1 (1 row) + +starting permutation: s2ip2 s1brr s1ifp2 s2brr s2dp2 s1c s2c +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1ifp2: INSERT INTO fk_parted_pk VALUES (2); +step s2brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s2dp2: DELETE FROM pk_noparted WHERE a = 2; +step s1c: COMMIT; +step s2dp2: <... completed> +ERROR: could not serialize access due to concurrent update +step s2c: COMMIT; + +starting permutation: s2ip2 s1brr s1ifn2 s2brr s2dp2 s1c s2c +step s2ip2: INSERT INTO pk_noparted VALUES (2); +step s1brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s1ifn2: INSERT INTO fk_noparted_sn VALUES (2); +step s2brr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s2dp2: DELETE FROM pk_noparted WHERE a = 2; +step s1c: COMMIT; +step s2dp2: <... completed> +ERROR: could not serialize access due to concurrent update +step s2c: COMMIT; diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec index 378507fbc3..9fad57e768 100644 --- a/src/test/isolation/specs/fk-snapshot.spec +++ b/src/test/isolation/specs/fk-snapshot.spec @@ -13,6 +13,11 @@ setup CREATE TABLE fk_noparted ( a int REFERENCES fk_parted_pk ON DELETE NO ACTION INITIALLY DEFERRED ); + + CREATE TABLE fk_noparted_sn ( + a int REFERENCES pk_noparted ON DELETE SET NULL + ); + INSERT INTO pk_noparted VALUES (1); INSERT INTO fk_parted_pk VALUES (1); INSERT INTO fk_noparted VALUES (1); @@ -20,7 +25,7 @@ setup teardown { - DROP TABLE pk_noparted, fk_parted_pk, fk_noparted; + DROP TABLE pk_noparted, fk_parted_pk, fk_noparted, fk_noparted_sn; } session s1 @@ -28,6 +33,7 @@ step s1brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } step s1brc { BEGIN ISOLATION LEVEL READ COMMITTED; } step s1ifp2 { INSERT INTO fk_parted_pk VALUES (2); } step s1ifp1 { INSERT INTO fk_parted_pk VALUES (1); } +step s1ifn2 { INSERT INTO fk_noparted_sn VALUES (2); } step s1dfp { DELETE FROM fk_parted_pk WHERE a = 1; } step s1c { COMMIT; } step s1sfp { SELECT * FROM fk_parted_pk; } @@ -38,6 +44,7 @@ session s2 step s2brr { BEGIN ISOLATION LEVEL REPEATABLE READ; } step s2brc { BEGIN ISOLATION LEVEL READ COMMITTED; } step s2ip2 { INSERT INTO pk_noparted VALUES (2); } +step s2dp2 { DELETE FROM pk_noparted WHERE a = 2; } step s2ifn2 { INSERT INTO fk_noparted VALUES (2); } step s2c { COMMIT; } step s2sfp { SELECT * FROM fk_parted_pk; } @@ -59,3 +66,11 @@ permutation s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 # the same no matter the snapshot mode permutation s1brr s1dfp s1ifp1 s1c s1sfn permutation s1brc s1dfp s1ifp1 s1c s1sfn + +# trying to delete a row through DELETE CASCADE, whilst that row is deleted +# in a concurrent transaction +permutation s2ip2 s1brr s1ifp2 s2brr s2dp2 s1c s2c + +# trying to update a row through DELETE SET NULL, whilst that row is deleted +# in a concurrent transaction +permutation s2ip2 s1brr s1ifn2 s2brr s2dp2 s1c s2c