v3-0001-Fix-assert-failures-in-parallel-SERIALIZABLE-READ.patch

From a3f34a9db20a455e0db47a5936ce10dd86d6f479 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Mon, 26 Jul 2021 17:57:47 +1200
Subject: [PATCH v3] Fix assert failures in parallel SERIALIZABLE READ ONLY.

1.  Make sure that we don't decrement SxactGlobalXminCount twice when
the SXACT_FLAG_RO_SAFE optimization is reached in a parallel query.
This could trigger a sanity check failure in assert builds.  Release
builds recompute the count in SetNewSxactGlobalXmin(), hiding the
problem.  Add a new isolation test for that case.

2.  Remove an assertion that the DOOMED flag can't be set on a partially
released SERIALIZABLEXACT.  Instead, ignore the flag (our transaction
was already determined to be read-only safe, and DOOMED is only set
incidentally during partial release).  Improve an existing isolation test
so that it reaches that case.

Back-patch to 12.  Bug #17116.  Defects in commit 47a338cf.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/17116-d6ca217acc180e30%40postgresql.org
---
 src/backend/storage/lmgr/predicate.c          | 20 +++-
 .../expected/serializable-parallel-2.out      | 57 +++--------
 .../expected/serializable-parallel-3.out      | 97 +++++++++++++++++++
 src/test/isolation/isolation_schedule         |  1 +
 .../specs/serializable-parallel-2.spec        | 12 ++-
 .../specs/serializable-parallel-3.spec        | 47 +++++++++
 6 files changed, 184 insertions(+), 50 deletions(-)
 create mode 100644 src/test/isolation/expected/serializable-parallel-3.out
 create mode 100644 src/test/isolation/specs/serializable-parallel-3.spec

diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 56267bdc3c..41688d8078 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -3331,6 +3331,7 @@ SetNewSxactGlobalXmin(void)
 void
 ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
 {
+	bool		partiallyReleasing = false;
 	bool		needToClear;
 	RWConflict	conflict,
 				nextConflict,
@@ -3431,6 +3432,7 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
 		else
 		{
 			MySerializableXact->flags |= SXACT_FLAG_PARTIALLY_RELEASED;
+			partiallyReleasing = true;
 			/* ... and proceed to perform the partial release below. */
 		}
 	}
@@ -3681,9 +3683,15 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
 	 * serializable transactions completes.  We then find the "new oldest"
 	 * xmin and purge any transactions which finished before this transaction
 	 * was launched.
+	 *
+	 * For parallel queries in read-only transactions, it might run twice.
+	 * We only release the reference on the first call.
 	 */
 	needToClear = false;
-	if (TransactionIdEquals(MySerializableXact->xmin, PredXact->SxactGlobalXmin))
+	if ((partiallyReleasing ||
+		 !SxactIsPartiallyReleased(MySerializableXact)) &&
+		TransactionIdEquals(MySerializableXact->xmin,
+							PredXact->SxactGlobalXmin))
 	{
 		Assert(PredXact->SxactGlobalXminCount > 0);
 		if (--(PredXact->SxactGlobalXminCount) == 0)
@@ -4839,10 +4847,14 @@ PreCommit_CheckForSerializationFailure(void)
 
 	LWLockAcquire(SerializableXactHashLock, LW_EXCLUSIVE);
 
-	/* Check if someone else has already decided that we need to die */
-	if (SxactIsDoomed(MySerializableXact))
+	/*
+	 * Check if someone else has already decided that we need to die.  Since
+	 * we set our own DOOMED flag when partially releasing, ignore in that
+	 * case.
+	 */
+	if (SxactIsDoomed(MySerializableXact) &&
+		!SxactIsPartiallyReleased(MySerializableXact))
 	{
-		Assert(!SxactIsPartiallyReleased(MySerializableXact));
 		LWLockRelease(SerializableXactHashLock);
 		ereport(ERROR,
 				(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
diff --git a/src/test/isolation/expected/serializable-parallel-2.out b/src/test/isolation/expected/serializable-parallel-2.out
index 92753ccf39..904fdd9080 100644
--- a/src/test/isolation/expected/serializable-parallel-2.out
+++ b/src/test/isolation/expected/serializable-parallel-2.out
@@ -1,50 +1,23 @@
 Parsed test spec with 2 sessions
 
 starting permutation: s1r s2r1 s1c s2r2 s2c
-step s1r: SELECT * FROM foo;
- a
---
- 1
- 2
- 3
- 4
- 5
- 6
- 7
- 8
- 9
-10
-(10 rows)
+step s1r: SELECT COUNT(*) FROM foo;
+count
+-----
+  100
+(1 row)
 
-step s2r1: SELECT * FROM foo;
- a
---
- 1
- 2
- 3
- 4
- 5
- 6
- 7
- 8
- 9
-10
-(10 rows)
+step s2r1: SELECT COUNT(*) FROM foo;
+count
+-----
+  100
+(1 row)
 
 step s1c: COMMIT;
-step s2r2: SELECT * FROM foo;
- a
---
- 1
- 2
- 3
- 4
- 5
- 6
- 7
- 8
- 9
-10
-(10 rows)
+step s2r2: SELECT COUNT(*) FROM foo;
+count
+-----
+  100
+(1 row)
 
 step s2c: COMMIT;
diff --git a/src/test/isolation/expected/serializable-parallel-3.out b/src/test/isolation/expected/serializable-parallel-3.out
new file mode 100644
index 0000000000..654276a385
--- /dev/null
+++ b/src/test/isolation/expected/serializable-parallel-3.out
@@ -0,0 +1,97 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s1r s3r s2r1 s4r1 s1c s2r2 s3c s4r2 s4c s2c
+step s1r: SELECT * FROM foo;
+ a
+--
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+10
+(10 rows)
+
+step s3r: SELECT * FROM foo;
+ a
+--
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+10
+(10 rows)
+
+step s2r1: SELECT * FROM foo;
+ a
+--
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+10
+(10 rows)
+
+step s4r1: SELECT * FROM foo;
+ a
+--
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+10
+(10 rows)
+
+step s1c: COMMIT;
+step s2r2: SELECT * FROM foo;
+ a
+--
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+10
+(10 rows)
+
+step s3c: COMMIT;
+step s4r2: SELECT * FROM foo;
+ a
+--
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+10
+(10 rows)
+
+step s4c: COMMIT;
+step s2c: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index f4c01006fc..67ef93b440 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -96,3 +96,4 @@ test: plpgsql-toast
 test: truncate-conflict
 test: serializable-parallel
 test: serializable-parallel-2
+test: serializable-parallel-3
diff --git a/src/test/isolation/specs/serializable-parallel-2.spec b/src/test/isolation/specs/serializable-parallel-2.spec
index f3941f7863..c975d96d77 100644
--- a/src/test/isolation/specs/serializable-parallel-2.spec
+++ b/src/test/isolation/specs/serializable-parallel-2.spec
@@ -3,7 +3,8 @@
 
 setup
 {
-	CREATE TABLE foo AS SELECT generate_series(1, 10)::int a;
+	CREATE TABLE foo AS SELECT generate_series(1, 100)::int a;
+	CREATE INDEX ON foo(a);
 	ALTER TABLE foo SET (parallel_workers = 2);
 }
 
@@ -14,7 +15,7 @@ teardown
 
 session s1
 setup 		{ BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; }
-step s1r	{ SELECT * FROM foo; }
+step s1r	{ SELECT COUNT(*) FROM foo; }
 step s1c 	{ COMMIT; }
 
 session s2
@@ -22,9 +23,12 @@ setup		{
 			  BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY;
 			  SET parallel_setup_cost = 0;
 			  SET parallel_tuple_cost = 0;
+			  SET min_parallel_index_scan_size = 0;
+			  SET parallel_leader_participation = off;
+			  SET enable_seqscan = off;
 			}
-step s2r1	{ SELECT * FROM foo; }
-step s2r2	{ SELECT * FROM foo; }
+step s2r1	{ SELECT COUNT(*) FROM foo; }
+step s2r2	{ SELECT COUNT(*) FROM foo; }
 step s2c	{ COMMIT; }
 
 permutation s1r s2r1 s1c s2r2 s2c
diff --git a/src/test/isolation/specs/serializable-parallel-3.spec b/src/test/isolation/specs/serializable-parallel-3.spec
new file mode 100644
index 0000000000..c27298c24f
--- /dev/null
+++ b/src/test/isolation/specs/serializable-parallel-3.spec
@@ -0,0 +1,47 @@
+# Exercise the case where a read-only serializable transaction has
+# SXACT_FLAG_RO_SAFE set in a parallel query.  This variant is like
+# two copies of #2 running at the same time, and excercises the case
+# where another transaction has the same xmin, and it is the oldest.
+
+setup
+{
+	CREATE TABLE foo AS SELECT generate_series(1, 10)::int a;
+	ALTER TABLE foo SET (parallel_workers = 2);
+}
+
+teardown
+{
+	DROP TABLE foo;
+}
+
+session s1
+setup 		{ BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; }
+step s1r	{ SELECT * FROM foo; }
+step s1c 	{ COMMIT; }
+
+session s2
+setup		{
+			  BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY;
+			  SET parallel_setup_cost = 0;
+			  SET parallel_tuple_cost = 0;
+			}
+step s2r1	{ SELECT * FROM foo; }
+step s2r2	{ SELECT * FROM foo; }
+step s2c	{ COMMIT; }
+
+session s3
+setup		{ BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; }
+step s3r	{ SELECT * FROM foo; }
+step s3c	{ COMMIT; }
+
+session s4
+setup		{
+			  BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY;
+			  SET parallel_setup_cost = 0;
+			  SET parallel_tuple_cost = 0;
+			}
+step s4r1	{ SELECT * FROM foo; }
+step s4r2	{ SELECT * FROM foo; }
+step s4c	{ COMMIT; }
+
+permutation s1r s3r s2r1 s4r1 s1c s2r2 s3c s4r2 s4c s2c
-- 
2.30.2