From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Alexander Lakhin <exclusion@gmail.com>
To: pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
Subject: Re: BUG #17893: Assert failed in heap_update()/_delete() when FK modiified by RI trigger in non-read-committed xact
Date: Mon, 27 Nov 2023 21:23:37 +0200
Message-ID: <0c6e10cd-9288-4dda-b35e-7a64684367d3@iki.fi> (raw)
In-Reply-To: <1236e144-d772-7ff4-6e23-4ce91d0923ad@gmail.com>
References: <17893-35847009eec517b5@postgresql.org>
<b77078c0-090c-0f0e-6f13-3bb7daca93ee@gmail.com>
<1236e144-d772-7ff4-6e23-4ce91d0923ad@gmail.com>
On 23/10/2023 09:00, Alexander Lakhin wrote:
> Third, with this change applied I immediately got a failure of the next
> assert in heap_delete():
> [added more context to show the code better]
> 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));
> }
> }
Yeah that Assert is wrong and should be removed. I think it's trying to
assert that if we are deleting a tuple and the visibility check fails,
it must be because it was concurrently updated, not because the
inserting XID is not visible. But that doesn't hold for this additional
check with RI updates, the inserting XID might well be invisible to the
crosscheck snapshot.
>> which was introduced by 5db6df0c0.
> Sorry for my mistake here. I had cited a wrong line. It should be:
> Assert(result != TM_Updated ||
> !ItemPointerEquals(&tp.t_self, &tp.t_data->t_ctid));
> As I still can't see, which cases those asserts intended for, but see cases
> when they are falsified, I propose removing them.
> Please look at the complete patch attached.
I propose to attached slight variation, which moves the assertions
before the crosscheck snapshot check. The assertions are correct as they
are, if you don't need to take the crosscheck into account.
I'm a little worried if the callers of tuple_update() might also get
confused when tuple_update() returns TM_Updated but xmax is invalid. As
you said:
> So in the latter case the tuple's xmin is not committed according to the
> crosscheck snapshot. Meanwhile, a comment in nodeModifyTable.c says:
> * Note: if es_crosscheck_snapshot isn't InvalidSnapshot, we check that
> * the row to be updated is visible to that snapshot, and throw a
> * can't-serialize error if not. ...
>
>
> And with a non-assert build I really get:
> ERROR: could not serialize access due to concurrent update
> in these cases.
I think you only get that error with REPEATABLE READ or SERIALIZABLE
isolation level. I don't quite understand how this works. In READ
COMMITTED level, ISTM that ExecUpdate() or ExecDelete() will proceed
with EvalPlanQualSlot() and locking the row with table_tuple_lock(). Or
do we never use the cross-check snapshot in READ COMMITTED mode?
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:v2-0001-Fix-assertions-with-RI-triggers-in-heap_update-an.patch (text/x-patch, 7.0K) download
From d3f733ebb15c8df576b21e59d7c8582c781bb6a7 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 27 Nov 2023 21:17:54 +0200
Subject: [PATCH v2 1/1] 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.
Author: Alexander Lakhin
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.cindex 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.hindex dbb709b56c..1ebf5a74c3 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.outindex 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; <waiting ...>+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; <waiting ...>+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.specindex 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--
2.39.2
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: hlinnaka@iki.fi, exclusion@gmail.com, andres@anarazel.de
Subject: Re: BUG #17893: Assert failed in heap_update()/_delete() when FK modiified by RI trigger in non-read-committed xact
In-Reply-To: <0c6e10cd-9288-4dda-b35e-7a64684367d3@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox