From: Alexander Lakhin <exclusion@gmail.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Cc: heikki.linnakangas@iki.fi, Kevin Grittner <kgrittn@gmail.com>
Subject: Re: BUG #17116: Assert failed in SerialSetActiveSerXmin() on commit of parallelized serializable transaction
Date: Sun, 5 Mar 2023 12:00:01 +0300
Message-ID: <0ba7e4cc-6b81-aebe-b087-8f59c4248511@gmail.com> (raw)
In-Reply-To: <CA+hUKG+zrWbCX6MGm=XJtMH6PiChS5Pxn+JqyUTEwpozaaapow@mail.gmail.com>
References: <17116-d6ca217acc180e30@postgresql.org>
<CA+hUKGJCUnwUbydS8HSOdbjnrYKG+TtHe7wqyt58FGp71irnrw@mail.gmail.com>
<CA+hUKGJmSjkA4qp1ywKvZXc8EA8vW_rPbObzmowuYTo0eF910A@mail.gmail.com>
<66a6851c-e213-b31a-c8e6-789ab06e17f1@gmail.com>
<CA+hUKGKNQc9q+bUMyYWVKAuKgtrS-aY-AW0V1vzYm6FkMnYFqQ@mail.gmail.com>
<d1ff80d3-4191-0b30-d9ce-aa11b95c2d00@gmail.com>
<CA+hUKGJ+V_9CGwFRU3qtZObfcKu-pLWKhocfQd4ouiZExPd4ug@mail.gmail.com>
<CA+hUKG+uRkCpVCfrUb0rTMyEBLhfpsDuW91d9XNT1d-cxEJenQ@mail.gmail.com>
<CA+hUKGJd-=mdUdS+Gh-FN4rZgAg4M-V==G7FMCU-KbUffyPJDw@mail.gmail.com>
<09825f99-97e3-daa1-3d84-05d7263da947@gmail.com>
<CA+hUKG+zrWbCX6MGm=XJtMH6PiChS5Pxn+JqyUTEwpozaaapow@mail.gmail.com>
Hi Thomas,
15.01.2022 08:41, Thomas Munro wrote:
> On Mon, Jan 10, 2022 at 6:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>> As to v4-0001-Fix-assert-failures-in-parallel-SERIALIZABLE-READ.patch
>> you posted before, I believe it fixes the issues as expected and may be
>> committed at least to allow for advanced testing.
> Cool, I'll do that in the next few days. Need to swap SERIALIZABLE
> back into my neurons after some time away...
I've caught one of asserts we discussed here again (in a different setup)
and would like to continue working on the fixes.
I've rechecked
v4-0001-Fix-assert-failures-in-parallel-SERIALIZABLE-READ.patch
— it's still in a good shape, needs only minor corrections for the
master branch and can be applied as-is to REL_12_STABLE .. REL_15_STABLE.
The modified test serializable-parallel-2 and new test
serializable-parallel-3 still trigger asserts with unpatched predicate.c
and make check-world passes with the complete patch applied.
So if that patch requires attention of a seasoned reviewer, let's
send it to the commitfest (I can register the patch if you don't mind)?
Otherwise, maybe just commit it and move to other bugs...
Best regards,
Alexander
Attachments:v4-0001-Fix-assert-failures-in-parallel-SERIALIZABLE-READ-master.patch (text/x-patch, 8.6K) download
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.cindex 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;
SERIALIZABLEXACT *roXact;
dlist_mutable_iter iter;
@@ -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.outindex 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.outnew file mode 100644index 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_scheduleindex f4c01006fc..67ef93b440 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.specindex 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.specnew file mode 100644index 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
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, thomas.munro@gmail.com, pgsql-bugs@lists.postgresql.org, kgrittn@gmail.com
Subject: Re: BUG #17116: Assert failed in SerialSetActiveSerXmin() on commit of parallelized serializable transaction
In-Reply-To: <0ba7e4cc-6b81-aebe-b087-8f59c4248511@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