pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: 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, 23 Oct 2023 09:00:00 +0300
Message-ID: <1236e144-d772-7ff4-6e23-4ce91d0923ad@gmail.com> (raw )
In-Reply-To: <b77078c0-090c-0f0e-6f13-3bb7daca93ee@gmail.com >
References: <17893-35847009eec517b5@postgresql.org >
<b77078c0-090c-0f0e-6f13-3bb7daca93ee@gmail.com >
02.09.2023 16:00, Alexander Lakhin wrote:
>
> Third, with this change applied I immediately got a failure of the next
> assert in heap_delete():
> Assert(!ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid));
> 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.
Best regards,
Alexander
Attachments:
v1-01-fix-asserts-in-heap_update-delete.patch (text/x-patch, 5.5K) download
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 14de8158d4..b625a8e79d 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2721,9 +2721,8 @@ l1:
result == TM_Updated ||
result == TM_Deleted ||
result == TM_BeingModified);
- Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID));
- Assert(result != TM_Updated ||
- !ItemPointerEquals(&tp.t_self, &tp.t_data->t_ctid));
+ Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID) ||
+ (result == TM_Updated && crosscheck != InvalidSnapshot));
tmfd->ctid = tp.t_data->t_ctid;
tmfd->xmax = HeapTupleHeaderGetUpdateXid(tp.t_data);
if (result == TM_SelfModified)
@@ -3346,10 +3345,7 @@ l2:
{
/* 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));
- }
}
if (result != TM_Ok)
@@ -3358,9 +3354,8 @@ l2:
result == TM_Updated ||
result == TM_Deleted ||
result == TM_BeingModified);
- Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID));
- Assert(result != TM_Updated ||
- !ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid));
+ Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID) ||
+ (result == TM_Updated && crosscheck != InvalidSnapshot));
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 230bc39cc0..ac7b279f32 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; <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.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
view thread (6+ messages) latest in thread
Message-ID: <1236e144-d772-7ff4-6e23-4ce91d0923ad@gmail.com>
Permalink: ../1236e144-d772-7ff4-6e23-4ce91d0923ad@gmail.com/
Also on: postgresql.org/message-id/1236e144-d772-7ff4-6e23-4ce91d0923ad@gmail.com
copy link · copy postgr.es · copy git am
reply 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: 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: <1236e144-d772-7ff4-6e23-4ce91d0923ad@gmail.com>
* 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