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

@@ -2856,13 +2856,7 @@ l1:
 			result = TM_Deleted;
 	}
 
-	if (crosscheck != InvalidSnapshot && result == TM_Ok)
+	/* sanity check the result HeapTupleSatisfiesUpdate() and the logic above */
-	{
-		/* Perform additional check for transaction-snapshot mode RI updates */
-		if (!HeapTupleSatisfiesVisibility(&tp, crosscheck, buffer))
-			result = TM_Updated;
-	}
-
 	if (result != TM_Ok)
 	{
 		Assert(result == TM_SelfModified ||
@@ -2872,6 +2866,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)
@@ -3483,16 +3488,7 @@ l2:
 			result = TM_Deleted;
 	}
 
-	if (crosscheck != InvalidSnapshot && result == TM_Ok)
+	/* Sanity check the result HeapTupleSatisfiesUpdate() and the logic above */
-	{
-		/* 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)
 	{
 		Assert(result == TM_SelfModified ||
@@ -3502,6 +3498,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)

src/include/access/tableam.h

@@ -1455,8 +1455,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
+ * t_xmax, and, if possible, t_cmax.  See comments for struct
- * struct TM_FailureData for additional info.
+ * TM_FailureData for additional info.
  */
 static inline TM_Result
 table_tuple_delete(Relation rel, ItemPointer tid, CommandId cid,

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;

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