[prev in list] [next in list] [prev in thread] [next in thread]
List: pgsql-bugs
Subject: Re: BUG #17116: Assert failed in SerialSetActiveSerXmin() on commit of parallelized serializable tra
From: Thomas Munro <thomas.munro () gmail ! com>
Date: 2021-07-30 5:48:25
Message-ID: CA+hUKGJd-=mdUdS+Gh-FN4rZgAg4M-V==G7FMCU-KbUffyPJDw () mail ! gmail ! com
[Download RAW message or body]
On Fri, Jul 30, 2021 at 9:41 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Wed, Jul 28, 2021 at 5:02 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > On Wed, Jul 28, 2021 at 9:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> > > (Except for read-only-anomaly-3, that causes another
> > > assertion fail but I believe that it's not related to the initial issue
> > > and deserves another bug report.)
The problem is that if we don't take the fast exit here:
if (XactReadOnly && PredXact->WritableSxactCount == 0)
{
ReleasePredXact(sxact);
LWLockRelease(SerializableXactHashLock);
return snapshot;
}
... then we search for writable SSI transactions here:
for (othersxact = FirstPredXact();
othersxact != NULL;
othersxact = NextPredXact(othersxact))
{
if (!SxactIsCommitted(othersxact)
&& !SxactIsDoomed(othersxact)
&& !SxactIsReadOnly(othersxact))
{
SetPossibleUnsafeConflict(sxact, othersxact);
}
}
... but that can find nothing if all writers happen to be doomed.
WritableSxactCount doesn't count read-only or committed transactions
so we don't have to worry about those confusing us, but it does count
doomed transactions. Having an empty list here is mostly fine, except
that nobody will ever set our RO_SAFE flag, and in the case of
DEFERRABLE, the assertion that someone has set it will fail. This is
the case since:
===
commit bdaabb9b22caa71021754d3967b4032b194d9880
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri Jul 8 00:36:30 2011 +0300
There's a small window wherein a transaction is committed but not yet
on the finished list, and we shouldn't flag it as a potential conflict
if so. We can also skip adding a doomed transaction to the list of
possible conflicts because we know it won't commit.
Dan Ports and Kevin Grittner.
===
I see three fixes:
1. Check if the list is empty, and if so, set our own
SXACT_FLAG_RO_SAFE. Then the assertion in GetSafeSnapshot() will
pass, and also non-DEFERRABLE callers will eventually see the flag and
opt out.
2. Check if the list is empty, and if so, opt out immediately (as we
do when WriteableSxactCount == 0). This would be strictly better than
#1, because it happens sooner while we still have the lock. On the
other hand, we'd have to undo more effects, and that involves writing
more fragile and very rarely run code.
3. Just delete the assertion in GetSafeSnapshot(). This is
attractively simple but it means that READ ONLY non-DEFERRABLE
transactions arbitrarily miss out on the RO_SAFE optimisation in this
(admittedly rare) case.
The attached 0002 has idea #1, which I prefer for back-patching. I
think #2 might be a good idea if we take the filtering logic further
so that it becomes more likely that you get an empty list (for
example, why don't we skip transactions that are running in a
different database?), but then that'd be new code, not something we
back-patch. Thoughts?
If someone is wanting to reproduce this case, try removing everything
from isolation_schedule except read-only-anomaly* and
read-write-unique*, and then run something like this while you go out
for lunch. It should work as far back as 11 (before that, those tests
didn't exist and the isolation tester couldn't run them due to lack of
handling for DEFERRABLE blocking):
rounds=1000
concurrency=100
output=/tmp/junk
for i in `seq $rounds` ; do
echo "=== round $i ==="
for c in `seq $concurrency` ; do
mkdir -p $output/results_${i}_${c}
(make -C src/test/isolation installcheck \
EXTRA_REGRESS_OPTS="--dbname=regress_${c}
--outputdir=$output/results_${i}_${c}" || echo FAIL) \
> junk/out.${i}.${c}.log 2>&1 &
pids[${c}]=$!
done
for c in `seq $concurrency` ; do
wait ${pids[${c}]}
done
done
["v4-0001-Fix-assert-failures-in-parallel-SERIALIZABLE-READ.patch" (text/x-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 v4 1/2] 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
["v4-0002-Fix-race-in-SERIALIZABLE-READ-ONLY.patch" (text/x-patch)]
From c24c7ca65f9ad5f777d5fa642c2d4f63020225eb Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Fri, 30 Jul 2021 17:27:16 +1200
Subject: [PATCH v4 2/2] Fix race in SERIALIZABLE READ ONLY.
Commit bdaabb9b skipped doomed transactions when building the list of
possible conflicts when obtaining a snapshot for SERIALIZABLE READ ONLY.
This had two unintended consequences:
1. With unlucky timing, a transaction will never benefit from the
SXACT_FLAG_RO_SAFE optimization, because no one will ever set the flag.
2. In the same circumstances but with DEFERRABLE, an assertion that
SXACT_FLAG_RO_SAFE has been set fails.
Go ahead and set the flag immediately if the list turns out to be empty.
(We could also free the SERIALIZABLEXACT immediately, but this change
seems simpler.)
Back-patch to all supported releases. Discovered along with bug #17116,
but a separate commit to back-patch further.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/17116-d6ca217acc180e30%40postgresql.org
---
src/backend/storage/lmgr/predicate.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 41688d8078..93632f1522 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -1900,6 +1900,13 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
SetPossibleUnsafeConflict(sxact, othersxact);
}
}
+
+ /*
+ * If we didn't find any possibly unsafe conflicts. Set our own
+ * SXACT_FLAG_RO_SAFE flag immediately, because no one else will.
+ */
+ if (SHMQueueEmpty(&sxact->possibleUnsafeConflicts))
+ sxact->flags |= SXACT_FLAG_RO_SAFE;
}
else
{
--
2.30.2
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic