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.  Non-assert
builds recompute the count in SetNewSxactGlobalXmin(), so the problem
was hidden, explaining the lack of field reports.  Add a new isolation
test to exercise 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 in fact set
during partial release, and there was already an assertion that it
wasn't set sooner).  Improve an existing isolation test so that it
reaches that case (previously it wasn't quite testing what it was
supposed to be testing; see discussion).

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

6 files changed, 184 insertions, 50 deletions

diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 0f7f7ba5f11..6b5a416873b 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -3232,6 +3232,7 @@ SetNewSxactGlobalXmin(void)
 void
 ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
 {
+	bool		partiallyReleasing = false;
 	bool		needToClear;
 	SERIALIZABLEXACT *roXact;
 	dlist_mutable_iter iter;
@@ -3330,6 +3331,7 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
 		else
 		{
 			MySerializableXact->flags |= SXACT_FLAG_PARTIALLY_RELEASED;
+			partiallyReleasing = true;
 			/* ... and proceed to perform the partial release below. */
 		}
 	}
@@ -3548,9 +3550,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)
@@ -4624,10 +4632,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 92753ccf39f..904fdd90806 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 00000000000..654276a3856
--- /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 c11dc9a4202..4fc56ae99c9 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -108,4 +108,5 @@ test: cluster-conflict-partition
 test: truncate-conflict
 test: serializable-parallel
 test: serializable-parallel-2
+test: serializable-parallel-3
 test: matview-write-skew
diff --git a/src/test/isolation/specs/serializable-parallel-2.spec b/src/test/isolation/specs/serializable-parallel-2.spec
index f3941f78631..c975d96d772 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 00000000000..c27298c24ff
--- /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