pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFix race condition in SSI when reading PredXact->SxactGlobalXmin
11+ messages / 4 participants
[nested] [flat]
* Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2025-09-04 22:09 Josh Curtis <jcurtis825@gmail.com>
0 siblings, 2 replies; 11+ messages in thread
From: Josh Curtis @ 2025-09-04 22:09 UTC (permalink / raw)
To: pgsql-hackers
DropAllPredicateLocksFromTable, PredicateLockPageSplit,
and CheckTableForSerializableConflictIn all assume that if
PredXact->SxactGlobalXmin is invalid there are no active serializable
transactions and that they can return early as there's no work to do. They
don't acquire a LWLock on SerializableXactHashLock because they already
have locks on the relevant buffer page or relation, which should protect
them from conflicts with new concurrent transactions. However, this
justification doesn't protect against concurrent updates of SxactGlobalXmin
which SetNewSxactGlobalXmin can do.
SetNewSxactGlobalXmin sets SxactGlobalXmin to InvalidTransactionId then
iterates over the active serializable transactions searching for a new
xmin.
static void
SetNewSxactGlobalXmin(void)
{
...
PredXact->SxactGlobalXmin = InvalidTransactionId;
PredXact->SxactGlobalXminCount = 0;
dlist_foreach(iter, &PredXact->activeList)
{
// find new xmin
}
...
}
If SetNewSxactGlobalXmin is interrupted before a new xmin has been set,
processes that read SxactGlobalXmin during this time will see
InvalidTransactionId and might incorrectly conclude there are no concurrent
serializable transactions. In the case of PredicateLockPageSplit, it will
return early and fail to transfer over information about SIRead locks on
the old page as a result.
I have a small repo that demonstrates serialization anomalies when
PredicateLockPageSplit is incorrectly skipped (they will show up much more
frequently with a small sleep after SetNewSxactGlobalXmin sets
SxactGlobalXmin to invalid, but they still show up eventually on normal
builds). [1]
I see a couple of options to fix this
1. Guard the reads with a lock. I've attached a patch to do this.
2. Update SetNewSxactGlobalXmin so concurrent lockless reads
of SxactGlobalXmin will never see an invalid transaction ID unless there
genuinely are no more active serializable transactions. To do this, delay
assigning PredXact->SxactGlobalXmin until all active transactions have been
examined.
static void
SetNewSxactGlobalXmin(void)
{
...
Assert(TransactionIdIsValid(PredXact->SxactGlobalXmin))
TransactionId xmin = InvalidTransactionId;
....
dlist_foreach(iter, &PredXact->activeList)
{
// iterate over transactions and find new xmin
}
PredXact->SxactGlobalXmin = xmin;
....
}
This is definitely a bit more complex. It requires that
SetNewSxactGlobalXmin is never called when SxactGlobalXmin is invalid to
prevent readers from seeing an invalid transaction ID when they should see
a valid one -- I think this is the case now since before
SetNewSxactGlobalXmin is called postgres checks that
PredXact->SxactGlobalXminCount > 0. I assume this entails that
SxactGlobalXmin is valid, but I have not checked every place the two
variables are modified.
I imagine the vast majority of postgres instances aren't using serializable
isolation, so I figured I'd throw this out as a possible optimization. I'm
not sure how the project trades off between simplicity and (very minor?)
performance optimizations, so I'll defer to the community.
Best,
Josh Curtis
[1] https://github.com/joshcurtis/ssi_race_condition_reproduction
Attachments:
v1-0001-Fix-race-condition-when-reading-PredXact-SxactGlo.patch (application/octet-stream, 3.4K) download
From a05d9741c3161a894decd1b88e148c4bbac60b0e Mon Sep 17 00:00:00 2001
From: Josh Curtis <jcurtis825@gmail.com>
Date: Mon, 1 Sep 2025 21:07:52 -0700
Subject: [PATCH v1] Fix race condition when reading
`PredXact->SxactGlobalXmin`
`DropAllPredicateLocksFromTable`, `PredicateLockPageSplit`, and
`CheckTableForSerializableConflictIn` all assume that
`!TransactionIdIsValid(PredXact->SxactGlobalXmin)` implies there are
no active serializable transactions.
This assumption is not true due to a race with
`SetNewSxactGlobalXmin`, which first sets `PredXact->SxactGlobalXmin`
to `InvalidTransactionId` then iterates over the active serializable
transactions to find the new xmin. If `SetNewSxactGlobalXmin` is
interrupted before setting a new xmin and other processes check
`!TransactionIdIsValid(PredXact->SxactGlobalXmin)` without taking a
lock during this time, they will incorrectly assume it is safe to
proceed as if there are no concurrent serializable transactions.
---
src/backend/storage/lmgr/predicate.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index c1d8511ad17..50e9f62b566 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -2949,12 +2949,14 @@ DropAllPredicateLocksFromTable(Relation relation, bool transfer)
/*
* Bail out quickly if there are no serializable transactions running.
- * It's safe to check this without taking locks because the caller is
- * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
- * would matter here can be acquired while that is held.
*/
+ LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ LWLockRelease(SerializableXactHashLock);
return;
+ }
+ LWLockRelease(SerializableXactHashLock);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -3150,16 +3152,14 @@ PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
/*
* Bail out quickly if there are no serializable transactions running.
- *
- * It's safe to do this check without taking any additional locks. Even if
- * a serializable transaction starts concurrently, we know it can't take
- * any SIREAD locks on the page being split because the caller is holding
- * the associated buffer page lock. Memory reordering isn't an issue; the
- * memory barrier in the LWLock acquisition guarantees that this read
- * occurs while the buffer page lock is held.
*/
+ LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ LWLockRelease(SerializableXactHashLock);
return;
+ }
+ LWLockRelease(SerializableXactHashLock);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -4426,12 +4426,14 @@ CheckTableForSerializableConflictIn(Relation relation)
/*
* Bail out quickly if there are no serializable transactions running.
- * It's safe to check this without taking locks because the caller is
- * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
- * would matter here can be acquired while that is held.
*/
+ LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ LWLockRelease(SerializableXactHashLock);
return;
+ }
+ LWLockRelease(SerializableXactHashLock);
if (!SerializationNeededForWrite(relation))
return;
--
2.49.0
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2025-10-07 08:19 Mihail Nikalayeu <mihailnikalayeu@gmail.com>
parent: Josh Curtis <jcurtis825@gmail.com>
1 sibling, 1 reply; 11+ messages in thread
From: Mihail Nikalayeu @ 2025-10-07 08:19 UTC (permalink / raw)
To: Josh Curtis <jcurtis825@gmail.com>; +Cc: pgsql-hackers
Hello!
Haven't checked the patch yet, but I'd recommend to add .spec reproducer
with injection_points to simulate the race in reproducible way.
Best regards,
Mikhail.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2025-10-20 21:30 Mihail Nikalayeu <mihailnikalayeu@gmail.com>
parent: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
0 siblings, 0 replies; 11+ messages in thread
From: Mihail Nikalayeu @ 2025-10-20 21:30 UTC (permalink / raw)
To: Josh Curtis <jcurtis825@gmail.com>; +Cc: pgsql-hackers
To prevent it being lost forever - I registered commitfest entry for it:
https://commitfest.postgresql.org/patch/6145/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2025-10-21 00:55 Mihail Nikalayeu <mihailnikalayeu@gmail.com>
parent: Josh Curtis <jcurtis825@gmail.com>
1 sibling, 1 reply; 11+ messages in thread
From: Mihail Nikalayeu @ 2025-10-21 00:55 UTC (permalink / raw)
To: Josh Curtis <jcurtis825@gmail.com>; +Cc: pgsql-hackers
Hello!
Josh Curtis <jcurtis825@gmail.com>:
> This is definitely a bit more complex. It requires that SetNewSxactGlobalXmin is never called when SxactGlobalXmin is invalid to prevent readers from seeing an invalid transaction ID when they should see a valid one -- I think this is the case now since before SetNewSxactGlobalXmin is called postgres checks that PredXact->SxactGlobalXminCount > 0. I assume this entails that SxactGlobalXmin is valid, but I have not checked every place the two variables are modified.
Such logic feels fragile to me. Maybe add a special flag like
'PredXact->SxactGlobalSkipAllowed' which will be updated to 'true' by
SetNewSxactGlobalXmin, and dropped to 'false' by functions affecting
PredXact->activeList?
But I prefer the first solution anyway.
Best regards,
Mikhail.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2025-10-22 03:07 Josh Curtis <jcurtis825@gmail.com>
parent: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Josh Curtis @ 2025-10-22 03:07 UTC (permalink / raw)
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>; +Cc: pgsql-hackers
Thanks for taking a look.
I tried adding a reproduction with injection points, but had some trouble
with them. I'll give it another go this weekend.
I guess one of the commitfest entries should be deleted? I had already made
one already. I don't see a way to do that though.
https://commitfest.postgresql.org/patch/6037/
Josh
On Mon, Oct 20, 2025 at 5:55 PM Mihail Nikalayeu <mihailnikalayeu@gmail.com>
wrote:
> Hello!
>
> Josh Curtis <jcurtis825@gmail.com>:
> > This is definitely a bit more complex. It requires that
> SetNewSxactGlobalXmin is never called when SxactGlobalXmin is invalid to
> prevent readers from seeing an invalid transaction ID when they should see
> a valid one -- I think this is the case now since before
> SetNewSxactGlobalXmin is called postgres checks that
> PredXact->SxactGlobalXminCount > 0. I assume this entails that
> SxactGlobalXmin is valid, but I have not checked every place the two
> variables are modified.
>
> Such logic feels fragile to me. Maybe add a special flag like
> 'PredXact->SxactGlobalSkipAllowed' which will be updated to 'true' by
> SetNewSxactGlobalXmin, and dropped to 'false' by functions affecting
> PredXact->activeList?
> But I prefer the first solution anyway.
>
> Best regards,
> Mikhail.
>
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2026-02-21 21:15 Josh Curtis <jcurtis825@gmail.com>
parent: Josh Curtis <jcurtis825@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Josh Curtis @ 2026-02-21 21:15 UTC (permalink / raw)
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>; +Cc: pgsql-hackers
Here's a spec test with a reproduction if run against master.
I personally wouldn't merge the test into master. The logic is a bit
brittle and the test scope is quite narrow
On Tue, Oct 21, 2025 at 8:07 PM Josh Curtis <jcurtis825@gmail.com> wrote:
> Thanks for taking a look.
>
> I tried adding a reproduction with injection points, but had some trouble
> with them. I'll give it another go this weekend.
>
> I guess one of the commitfest entries should be deleted? I had already
> made one already. I don't see a way to do that though.
> https://commitfest.postgresql.org/patch/6037/
>
> Josh
>
> On Mon, Oct 20, 2025 at 5:55 PM Mihail Nikalayeu <
> mihailnikalayeu@gmail.com> wrote:
>
>> Hello!
>>
>> Josh Curtis <jcurtis825@gmail.com>:
>> > This is definitely a bit more complex. It requires that
>> SetNewSxactGlobalXmin is never called when SxactGlobalXmin is invalid to
>> prevent readers from seeing an invalid transaction ID when they should see
>> a valid one -- I think this is the case now since before
>> SetNewSxactGlobalXmin is called postgres checks that
>> PredXact->SxactGlobalXminCount > 0. I assume this entails that
>> SxactGlobalXmin is valid, but I have not checked every place the two
>> variables are modified.
>>
>> Such logic feels fragile to me. Maybe add a special flag like
>> 'PredXact->SxactGlobalSkipAllowed' which will be updated to 'true' by
>> SetNewSxactGlobalXmin, and dropped to 'false' by functions affecting
>> PredXact->activeList?
>> But I prefer the first solution anyway.
>>
>> Best regards,
>> Mikhail.
>>
>
Attachments:
v1-0001-Add-test-to-demonstrate-bug-in-SSI.patch (application/octet-stream, 8.9K) download
From 13f6f89bd1101cdf78564ea778d40b2ff68a9a76 Mon Sep 17 00:00:00 2001
From: Josh Curtis <jcurtis825@gmail.com>
Date: Fri, 20 Feb 2026 11:48:34 -0800
Subject: [PATCH v1] Add test to demonstrate bug in SSI
---
src/backend/storage/lmgr/predicate.c | 9 ++
src/test/modules/injection_points/Makefile | 1 +
.../expected/predicate-lock-page-split.out | 86 +++++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../specs/predicate-lock-page-split.spec | 92 +++++++++++++++++++
5 files changed, 189 insertions(+)
create mode 100644 src/test/modules/injection_points/expected/predicate-lock-page-split.out
create mode 100644 src/test/modules/injection_points/specs/predicate-lock-page-split.spec
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index fe75ead350..de8a84b7e8 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -212,6 +212,7 @@
#include "storage/proc.h"
#include "storage/procarray.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
@@ -3156,6 +3157,7 @@ PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
* memory barrier in the LWLock acquisition guarantees that this read
* occurs while the buffer page lock is held.
*/
+ INJECTION_POINT("predicate-lock-page-split", NULL);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
return;
@@ -3255,6 +3257,10 @@ SetNewSxactGlobalXmin(void)
PredXact->SxactGlobalXmin = InvalidTransactionId;
PredXact->SxactGlobalXminCount = 0;
+#ifdef USE_INJECTION_POINTS
+ INJECTION_POINT_CACHED("predicate-set-sxact-global-xmin-invalid", NULL);
+#endif
+
dlist_foreach(iter, &PredXact->activeList)
{
SERIALIZABLEXACT *sxact =
@@ -3368,6 +3374,9 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
Assert(LocalPredicateLockHash == NULL);
return;
}
+#ifdef USE_INJECTION_POINTS
+ INJECTION_POINT_LOAD("predicate-set-sxact-global-xmin-invalid");
+#endif
LWLockAcquire(SerializableXactHashLock, LW_EXCLUSIVE);
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index a41d781f8c..73ed09eaa2 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -14,6 +14,7 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
inplace \
+ predicate-lock-page-split \
syscache-update-pruned \
heap_lock_update
diff --git a/src/test/modules/injection_points/expected/predicate-lock-page-split.out b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
new file mode 100644
index 0000000000..b0a6493519
--- /dev/null
+++ b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
@@ -0,0 +1,86 @@
+Parsed test spec with 5 sessions
+
+starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2_then_s1 s3_insert s3_commit s2_commit verify_order
+step s1_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) FROM test_table;
+
+max
+---
+406
+(1 row)
+
+step bump_xmin:
+ CREATE TABLE tmp_bump_xmin();
+ DROP TABLE tmp_bump_xmin;
+
+step s2_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) FROM test_table;
+
+max
+---
+406
+(1 row)
+
+step s3_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) FROM test_table;
+
+max
+---
+406
+(1 row)
+
+step s1_insert:
+ INSERT INTO test_table SELECT max(id) + 1 FROM test_table;
+
+step s2_insert_wait_at_page_split:
+ SELECT FROM injection_points_set_local();
+ SELECT FROM injection_points_attach('predicate-lock-page-split', 'wait');
+ INSERT INTO test_table SELECT max(id) + 1 FROM test_table;
+ <waiting ...>
+step s1_commit_wait_in_SetNewSxactGlobalXmin:
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('predicate-set-sxact-global-xmin-invalid', 'wait');
+ COMMIT;
+ <waiting ...>
+step wakeup_s2_then_s1:
+ SELECT FROM injection_points_wakeup('predicate-lock-page-split');
+ SELECT FROM injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
+ <waiting ...>
+step s2_insert_wait_at_page_split: <... completed>
+step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed>
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s3_insert:
+ INSERT INTO test_table SELECT max(id) + 1 FROM test_table;
+
+step wakeup_s2_then_s1: <... completed>
+step s3_commit:
+ COMMIT;
+
+step s2_commit:
+ COMMIT;
+
+ERROR: could not serialize access due to read/write dependencies among transactions
+step verify_order:
+ select id from test_table order by id desc limit 5;
+
+ id
+---
+407
+407
+406
+405
+404
+(5 rows)
+
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index fcc8541451..6ff7005a9b 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -45,6 +45,7 @@ tests += {
'specs': [
'basic',
'inplace',
+ 'predicate-lock-page-split',
'syscache-update-pruned',
'heap_lock_update',
],
diff --git a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
new file mode 100644
index 0000000000..1566090272
--- /dev/null
+++ b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
@@ -0,0 +1,92 @@
+# Test for race condition in PredicateLockPageSplit
+#
+# When SetNewSxactGlobalXmin() temporarily sets SxactGlobalXmin to
+# InvalidTransactionId, a concurrent PredicateLockPageSplit() will skip
+# transferring SIREAD locks to the new page. This lets a third transaction
+# insert onto the new page without SSI detecting the conflict.
+setup
+{
+ CREATE EXTENSION IF NOT EXISTS injection_points;
+ DROP TABLE IF EXISTS test_table;
+
+ CREATE TABLE test_table (id int);
+ CREATE INDEX test_table_id_idx ON test_table USING btree (id);
+ INSERT INTO test_table SELECT i FROM generate_series(1, 406) i;
+}
+
+
+teardown
+{
+ DROP TABLE IF EXISTS test_table;
+ DROP EXTENSION IF EXISTS injection_points;
+}
+
+session s0
+step bump_xmin {
+ CREATE TABLE tmp_bump_xmin();
+ DROP TABLE tmp_bump_xmin;
+}
+step verify_order {
+ select id from test_table order by id desc limit 5;
+}
+session s1
+step s1_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) FROM test_table;
+}
+step s1_insert {
+ INSERT INTO test_table SELECT max(id) + 1 FROM test_table;
+}
+step s1_commit_wait_in_SetNewSxactGlobalXmin {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('predicate-set-sxact-global-xmin-invalid', 'wait');
+ COMMIT;
+}
+
+session s2
+step s2_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) FROM test_table;
+}
+step s2_insert_wait_at_page_split {
+ SELECT FROM injection_points_set_local();
+ SELECT FROM injection_points_attach('predicate-lock-page-split', 'wait');
+ INSERT INTO test_table SELECT max(id) + 1 FROM test_table;
+}
+step s2_commit {
+ COMMIT;
+}
+
+session s3
+step s3_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) FROM test_table;
+}
+step s3_insert {
+ INSERT INTO test_table SELECT max(id) + 1 FROM test_table;
+}
+step s3_commit {
+ COMMIT;
+}
+
+session s4
+step wakeup_s2_then_s1 {
+ SELECT FROM injection_points_wakeup('predicate-lock-page-split');
+ SELECT FROM injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
+}
+
+# s1_begin: Start transaction 1, read max(id) = 406
+# bump_xmin: Advance xmin so s2 and s3 get a higher xmin than s1
+# s2_begin, s3_begin: Start transactions 2 and 3, read max(id) = 406
+#
+# at this point s1, s2, and s3 are all concurrent
+#
+# s1_insert: s1 inserts 407 into index page 1
+# s2_insert_wait_at_page_split: s2 inserts 407, triggers a page split and waits at the start of PredicateLockPageSplit
+# s1_commit_wait_in_SetNewSxactGlobalXmin: s1 commits then waits after setting SxactGlobalXmin to InvalidTransactionId in SetNewSxactGlobalXmin
+# wakeup_s2_then_s1: s2 wakes up, sees InvalidTransactionId, and skips transferring SIREAD locks to the new page
+# then s1 wakes up and finishes committing
+# s3_insert: s3 inserts 407 into index page 2 (no SIREAD locks were transferred = bug)
+# s3_commit: s3 commits successfully (should have been aborted)
+# s2_commit: s2 tries to commit but aborts due to serialization failure (cycle with s1)
+permutation s1_begin bump_xmin s2_begin s3_begin s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2_then_s1(s2_insert_wait_at_page_split,s1_commit_wait_in_SetNewSxactGlobalXmin) s3_insert s3_commit s2_commit verify_order
--
2.51.2
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2026-03-08 19:21 Mihail Nikalayeu <mihailnikalayeu@gmail.com>
parent: Josh Curtis <jcurtis825@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Mihail Nikalayeu @ 2026-03-08 19:21 UTC (permalink / raw)
To: Josh Curtis <jcurtis825@gmail.com>; +Cc: pgsql-hackers
Hello, Josh!
> Here's a spec test with a reproduction if run against master.
>
> I personally wouldn't merge the test into master. The logic is a bit brittle and the test scope is quite narrow
I refactored your test a little bit, now for my taste it feels committable.
Best regards,
Mikhail.
Attachments:
v2-0001-PATCH-v2-Add-test-to-demonstrate-bug-in-SSI.patch (application/octet-stream, 9.7K) download
From c99597016af620c6f527e06b445bed3c04158952 Mon Sep 17 00:00:00 2001
From: Mikhail Nikalayeu <mihailnikalayeu@gmail.com>
Date: Sun, 8 Mar 2026 19:20:02 +0100
Subject: [PATCH v2] [PATCH v2] Add test to demonstrate bug in SSI
---
src/backend/storage/lmgr/predicate.c | 9 ++
src/test/modules/injection_points/Makefile | 1 +
.../expected/predicate-lock-page-split.out | 105 ++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../specs/predicate-lock-page-split.spec | 128 ++++++++++++++++++
5 files changed, 244 insertions(+)
create mode 100644 src/test/modules/injection_points/expected/predicate-lock-page-split.out
create mode 100644 src/test/modules/injection_points/specs/predicate-lock-page-split.spec
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 548b4f66470..7eef2ca0418 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -212,6 +212,7 @@
#include "storage/proc.h"
#include "storage/procarray.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
#include "utils/wait_event.h"
@@ -3157,6 +3158,7 @@ PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
* memory barrier in the LWLock acquisition guarantees that this read
* occurs while the buffer page lock is held.
*/
+ INJECTION_POINT("predicate-lock-page-split", NULL);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
return;
@@ -3256,6 +3258,10 @@ SetNewSxactGlobalXmin(void)
PredXact->SxactGlobalXmin = InvalidTransactionId;
PredXact->SxactGlobalXminCount = 0;
+#ifdef USE_INJECTION_POINTS
+ INJECTION_POINT_CACHED("predicate-set-sxact-global-xmin-invalid", NULL);
+#endif
+
dlist_foreach(iter, &PredXact->activeList)
{
SERIALIZABLEXACT *sxact =
@@ -3369,6 +3375,9 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
Assert(LocalPredicateLockHash == NULL);
return;
}
+#ifdef USE_INJECTION_POINTS
+ INJECTION_POINT_LOAD("predicate-set-sxact-global-xmin-invalid");
+#endif
LWLockAcquire(SerializableXactHashLock, LW_EXCLUSIVE);
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index a41d781f8c9..73ed09eaa22 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -14,6 +14,7 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
inplace \
+ predicate-lock-page-split \
syscache-update-pruned \
heap_lock_update
diff --git a/src/test/modules/injection_points/expected/predicate-lock-page-split.out b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
new file mode 100644
index 00000000000..deeced73d88
--- /dev/null
+++ b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
@@ -0,0 +1,105 @@
+Parsed test spec with 5 sessions
+
+starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2_then_s1 s3_insert s3_commit s2_commit verify
+injection_points_attach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s1_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+
+locked
+------
+t
+(1 row)
+
+step bump_xmin:
+ DO $$
+ BEGIN
+ PERFORM pg_current_xact_id();
+ END;
+ $$;
+
+step s2_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+
+locked
+------
+t
+(1 row)
+
+step s3_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+
+locked
+------
+t
+(1 row)
+
+step s1_insert:
+ INSERT INTO test_table
+ SELECT max(id) + 1
+ FROM test_table;
+
+step s2_insert_wait_at_page_split:
+ DO $$
+ DECLARE
+ next_id int := (SELECT max(id) + 2 FROM test_table);
+ base_size bigint := pg_relation_size('test_table_id_idx');
+ BEGIN
+ LOOP
+ INSERT INTO test_table VALUES (next_id);
+ EXIT WHEN pg_relation_size('test_table_id_idx') > base_size;
+ next_id := next_id + 1;
+ END LOOP;
+ END;
+ $$;
+ <waiting ...>
+step s1_commit_wait_in_SetNewSxactGlobalXmin:
+ COMMIT;
+ <waiting ...>
+step wakeup_s2_then_s1:
+ SELECT injection_points_wakeup('predicate-lock-page-split');
+ SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
+ <waiting ...>
+step s2_insert_wait_at_page_split: <... completed>
+step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed>
+step s3_insert:
+ INSERT INTO test_table VALUES (1000000);
+
+ERROR: could not serialize access due to read/write dependencies among transactions
+step wakeup_s2_then_s1: <... completed>
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s3_commit:
+ COMMIT;
+
+step s2_commit:
+ COMMIT;
+
+ERROR: could not serialize access due to read/write dependencies among transactions
+step verify:
+ SELECT EXISTS (SELECT 1 FROM test_table WHERE id = 1000000) AS inserted;
+
+inserted
+--------
+f
+(1 row)
+
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index fcc85414515..6ff7005a9bf 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -45,6 +45,7 @@ tests += {
'specs': [
'basic',
'inplace',
+ 'predicate-lock-page-split',
'syscache-update-pruned',
'heap_lock_update',
],
diff --git a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
new file mode 100644
index 00000000000..bfd7ecc85c6
--- /dev/null
+++ b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
@@ -0,0 +1,128 @@
+# Test for race condition in PredicateLockPageSplit
+#
+# When SetNewSxactGlobalXmin() temporarily sets SxactGlobalXmin to
+# InvalidTransactionId, a concurrent PredicateLockPageSplit() can see
+# the invalid value and skip transferring SIREAD locks to the new page.
+# This lets a third transaction insert onto the new page without SSI
+# detecting the rw-conflict.
+#
+setup
+{
+ CREATE EXTENSION IF NOT EXISTS injection_points;
+ DROP TABLE IF EXISTS test_table;
+
+ CREATE TABLE test_table (id int);
+ CREATE INDEX test_table_id_idx ON test_table USING btree (id);
+ INSERT INTO test_table VALUES (1);
+}
+
+teardown
+{
+ DROP TABLE IF EXISTS test_table;
+ DROP EXTENSION IF EXISTS injection_points;
+}
+
+session s0
+step bump_xmin {
+ DO $$
+ BEGIN
+ PERFORM pg_current_xact_id();
+ END;
+ $$;
+}
+step verify {
+ SELECT EXISTS (SELECT 1 FROM test_table WHERE id = 1000000) AS inserted;
+}
+
+session s1
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('predicate-set-sxact-global-xmin-invalid', 'wait');
+}
+step s1_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+}
+step s1_insert {
+ INSERT INTO test_table
+ SELECT max(id) + 1
+ FROM test_table;
+}
+step s1_commit_wait_in_SetNewSxactGlobalXmin {
+ COMMIT;
+}
+
+session s2
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('predicate-lock-page-split', 'wait');
+}
+step s2_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+}
+step s2_insert_wait_at_page_split {
+ DO $$
+ DECLARE
+ next_id int := (SELECT max(id) + 2 FROM test_table);
+ base_size bigint := pg_relation_size('test_table_id_idx');
+ BEGIN
+ LOOP
+ INSERT INTO test_table VALUES (next_id);
+ EXIT WHEN pg_relation_size('test_table_id_idx') > base_size;
+ next_id := next_id + 1;
+ END LOOP;
+ END;
+ $$;
+}
+step s2_commit {
+ COMMIT;
+}
+
+session s3
+step s3_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+}
+step s3_insert {
+ INSERT INTO test_table VALUES (1000000);
+}
+step s3_commit {
+ COMMIT;
+}
+
+session s4
+step wakeup_s2_then_s1 {
+ SELECT injection_points_wakeup('predicate-lock-page-split');
+ SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
+}
+
+# s1_begin: s1 reads from the table, establishing SIREAD locks on the index
+# bump_xmin: advance xmin so s2/s3 get a higher xmin than s1
+# s2_begin, s3_begin: s2 and s3 read from the table (same snapshot as s1)
+#
+# s1_insert: s1 inserts max(id)+1
+# s2_insert_wait_at_page_split: s2 inserts ascending values until a real
+# btree page split happens, then waits in PredicateLockPageSplit before
+# checking SxactGlobalXmin
+# s1_commit_wait_in_SetNewSxactGlobalXmin: after s2 is already waiting,
+# s1 commits and waits after SetNewSxactGlobalXmin sets SxactGlobalXmin
+# to InvalidTransactionId
+# wakeup_s2_then_s1: wake s2 (sees InvalidTransactionId, skips SIREAD
+# lock transfer), then wake s1
+# s3_insert: s3 inserts 1000000 onto the new page (no SIREAD locks = bug)
+# s3_commit: s3 commits (should have been aborted by SSI)
+# s2_commit: s2 aborts due to serialization failure
+permutation
+ s1_begin
+ bump_xmin
+ s2_begin
+ s3_begin
+ s1_insert
+ s2_insert_wait_at_page_split
+ s1_commit_wait_in_SetNewSxactGlobalXmin
+ wakeup_s2_then_s1(s2_insert_wait_at_page_split,s1_commit_wait_in_SetNewSxactGlobalXmin)
+ s3_insert
+ s3_commit
+ s2_commit
+ verify
--
2.43.0
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2026-03-28 13:07 Mihail Nikalayeu <mihailnikalayeu@gmail.com>
parent: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Mihail Nikalayeu @ 2026-03-28 13:07 UTC (permalink / raw)
To: Josh Curtis <jcurtis825@gmail.com>; +Cc: pgsql-hackers
Hello!
Combined refined test and fix into a single commit to make cfbot happy.
Best regards,
Mikhail.
Attachments:
v3-0001-Fix-race-condition-when-reading-PredXact-SxactGlo.patch (application/octet-stream, 12.5K) download
From ab0b94adb78962d93194eb7ef470eade319bf534 Mon Sep 17 00:00:00 2001
From: Mikhail Nikalayeu <mihailnikalayeu@gmail.com>
Date: Sat, 28 Mar 2026 13:05:58 +0100
Subject: [PATCH v3] Fix race condition when reading
`PredXact->SxactGlobalXmin`
`DropAllPredicateLocksFromTable`, `PredicateLockPageSplit`, and
`CheckTableForSerializableConflictIn` all assume that
`!TransactionIdIsValid(PredXact->SxactGlobalXmin)` implies there are
no active serializable transactions.
This assumption is not true due to a race with
`SetNewSxactGlobalXmin`, which first sets `PredXact->SxactGlobalXmin`
to `InvalidTransactionId` then iterates over the active serializable
transactions to find the new xmin. If `SetNewSxactGlobalXmin` is
interrupted before setting a new xmin and other processes check
`!TransactionIdIsValid(PredXact->SxactGlobalXmin)` without taking a
lock during this time, they will incorrectly assume it is safe to
proceed as if there are no concurrent serializable transactions.
Author: Josh Curtis <jcurtis825@gmail.com>
Reviewed-by: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Discussion: https://postgr.es/m/flat/CAEkjoh2X8yL9TKrQKx6SL6gtMNuZsyH8K-%2BrE78EwdOv4oXjSA%40mail.gmail.com
---
src/backend/storage/lmgr/predicate.c | 37 +++--
src/test/modules/injection_points/Makefile | 1 +
.../expected/predicate-lock-page-split.out | 105 ++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../specs/predicate-lock-page-split.spec | 128 ++++++++++++++++++
5 files changed, 259 insertions(+), 13 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/predicate-lock-page-split.out
create mode 100644 src/test/modules/injection_points/specs/predicate-lock-page-split.spec
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index ae0e96aee5f..c0a1bc50df3 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -212,6 +212,7 @@
#include "storage/proc.h"
#include "storage/procarray.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
#include "utils/wait_event.h"
@@ -2960,12 +2961,14 @@ DropAllPredicateLocksFromTable(Relation relation, bool transfer)
/*
* Bail out quickly if there are no serializable transactions running.
- * It's safe to check this without taking locks because the caller is
- * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
- * would matter here can be acquired while that is held.
*/
+ LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ LWLockRelease(SerializableXactHashLock);
return;
+ }
+ LWLockRelease(SerializableXactHashLock);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -3161,16 +3164,15 @@ PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
/*
* Bail out quickly if there are no serializable transactions running.
- *
- * It's safe to do this check without taking any additional locks. Even if
- * a serializable transaction starts concurrently, we know it can't take
- * any SIREAD locks on the page being split because the caller is holding
- * the associated buffer page lock. Memory reordering isn't an issue; the
- * memory barrier in the LWLock acquisition guarantees that this read
- * occurs while the buffer page lock is held.
*/
+ INJECTION_POINT("predicate-lock-page-split", NULL);
+ LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ LWLockRelease(SerializableXactHashLock);
return;
+ }
+ LWLockRelease(SerializableXactHashLock);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -3268,6 +3270,10 @@ SetNewSxactGlobalXmin(void)
PredXact->SxactGlobalXmin = InvalidTransactionId;
PredXact->SxactGlobalXminCount = 0;
+#ifdef USE_INJECTION_POINTS
+ INJECTION_POINT_CACHED("predicate-set-sxact-global-xmin-invalid", NULL);
+#endif
+
dlist_foreach(iter, &PredXact->activeList)
{
SERIALIZABLEXACT *sxact =
@@ -3381,6 +3387,9 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
Assert(LocalPredicateLockHash == NULL);
return;
}
+#ifdef USE_INJECTION_POINTS
+ INJECTION_POINT_LOAD("predicate-set-sxact-global-xmin-invalid");
+#endif
LWLockAcquire(SerializableXactHashLock, LW_EXCLUSIVE);
@@ -4437,12 +4446,14 @@ CheckTableForSerializableConflictIn(Relation relation)
/*
* Bail out quickly if there are no serializable transactions running.
- * It's safe to check this without taking locks because the caller is
- * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
- * would matter here can be acquired while that is held.
*/
+ LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ LWLockRelease(SerializableXactHashLock);
return;
+ }
+ LWLockRelease(SerializableXactHashLock);
if (!SerializationNeededForWrite(relation))
return;
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index a41d781f8c9..73ed09eaa22 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -14,6 +14,7 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
inplace \
+ predicate-lock-page-split \
syscache-update-pruned \
heap_lock_update
diff --git a/src/test/modules/injection_points/expected/predicate-lock-page-split.out b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
new file mode 100644
index 00000000000..deeced73d88
--- /dev/null
+++ b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
@@ -0,0 +1,105 @@
+Parsed test spec with 5 sessions
+
+starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2_then_s1 s3_insert s3_commit s2_commit verify
+injection_points_attach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s1_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+
+locked
+------
+t
+(1 row)
+
+step bump_xmin:
+ DO $$
+ BEGIN
+ PERFORM pg_current_xact_id();
+ END;
+ $$;
+
+step s2_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+
+locked
+------
+t
+(1 row)
+
+step s3_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+
+locked
+------
+t
+(1 row)
+
+step s1_insert:
+ INSERT INTO test_table
+ SELECT max(id) + 1
+ FROM test_table;
+
+step s2_insert_wait_at_page_split:
+ DO $$
+ DECLARE
+ next_id int := (SELECT max(id) + 2 FROM test_table);
+ base_size bigint := pg_relation_size('test_table_id_idx');
+ BEGIN
+ LOOP
+ INSERT INTO test_table VALUES (next_id);
+ EXIT WHEN pg_relation_size('test_table_id_idx') > base_size;
+ next_id := next_id + 1;
+ END LOOP;
+ END;
+ $$;
+ <waiting ...>
+step s1_commit_wait_in_SetNewSxactGlobalXmin:
+ COMMIT;
+ <waiting ...>
+step wakeup_s2_then_s1:
+ SELECT injection_points_wakeup('predicate-lock-page-split');
+ SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
+ <waiting ...>
+step s2_insert_wait_at_page_split: <... completed>
+step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed>
+step s3_insert:
+ INSERT INTO test_table VALUES (1000000);
+
+ERROR: could not serialize access due to read/write dependencies among transactions
+step wakeup_s2_then_s1: <... completed>
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s3_commit:
+ COMMIT;
+
+step s2_commit:
+ COMMIT;
+
+ERROR: could not serialize access due to read/write dependencies among transactions
+step verify:
+ SELECT EXISTS (SELECT 1 FROM test_table WHERE id = 1000000) AS inserted;
+
+inserted
+--------
+f
+(1 row)
+
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index fcc85414515..6ff7005a9bf 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -45,6 +45,7 @@ tests += {
'specs': [
'basic',
'inplace',
+ 'predicate-lock-page-split',
'syscache-update-pruned',
'heap_lock_update',
],
diff --git a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
new file mode 100644
index 00000000000..bfd7ecc85c6
--- /dev/null
+++ b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
@@ -0,0 +1,128 @@
+# Test for race condition in PredicateLockPageSplit
+#
+# When SetNewSxactGlobalXmin() temporarily sets SxactGlobalXmin to
+# InvalidTransactionId, a concurrent PredicateLockPageSplit() can see
+# the invalid value and skip transferring SIREAD locks to the new page.
+# This lets a third transaction insert onto the new page without SSI
+# detecting the rw-conflict.
+#
+setup
+{
+ CREATE EXTENSION IF NOT EXISTS injection_points;
+ DROP TABLE IF EXISTS test_table;
+
+ CREATE TABLE test_table (id int);
+ CREATE INDEX test_table_id_idx ON test_table USING btree (id);
+ INSERT INTO test_table VALUES (1);
+}
+
+teardown
+{
+ DROP TABLE IF EXISTS test_table;
+ DROP EXTENSION IF EXISTS injection_points;
+}
+
+session s0
+step bump_xmin {
+ DO $$
+ BEGIN
+ PERFORM pg_current_xact_id();
+ END;
+ $$;
+}
+step verify {
+ SELECT EXISTS (SELECT 1 FROM test_table WHERE id = 1000000) AS inserted;
+}
+
+session s1
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('predicate-set-sxact-global-xmin-invalid', 'wait');
+}
+step s1_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+}
+step s1_insert {
+ INSERT INTO test_table
+ SELECT max(id) + 1
+ FROM test_table;
+}
+step s1_commit_wait_in_SetNewSxactGlobalXmin {
+ COMMIT;
+}
+
+session s2
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('predicate-lock-page-split', 'wait');
+}
+step s2_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+}
+step s2_insert_wait_at_page_split {
+ DO $$
+ DECLARE
+ next_id int := (SELECT max(id) + 2 FROM test_table);
+ base_size bigint := pg_relation_size('test_table_id_idx');
+ BEGIN
+ LOOP
+ INSERT INTO test_table VALUES (next_id);
+ EXIT WHEN pg_relation_size('test_table_id_idx') > base_size;
+ next_id := next_id + 1;
+ END LOOP;
+ END;
+ $$;
+}
+step s2_commit {
+ COMMIT;
+}
+
+session s3
+step s3_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+}
+step s3_insert {
+ INSERT INTO test_table VALUES (1000000);
+}
+step s3_commit {
+ COMMIT;
+}
+
+session s4
+step wakeup_s2_then_s1 {
+ SELECT injection_points_wakeup('predicate-lock-page-split');
+ SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
+}
+
+# s1_begin: s1 reads from the table, establishing SIREAD locks on the index
+# bump_xmin: advance xmin so s2/s3 get a higher xmin than s1
+# s2_begin, s3_begin: s2 and s3 read from the table (same snapshot as s1)
+#
+# s1_insert: s1 inserts max(id)+1
+# s2_insert_wait_at_page_split: s2 inserts ascending values until a real
+# btree page split happens, then waits in PredicateLockPageSplit before
+# checking SxactGlobalXmin
+# s1_commit_wait_in_SetNewSxactGlobalXmin: after s2 is already waiting,
+# s1 commits and waits after SetNewSxactGlobalXmin sets SxactGlobalXmin
+# to InvalidTransactionId
+# wakeup_s2_then_s1: wake s2 (sees InvalidTransactionId, skips SIREAD
+# lock transfer), then wake s1
+# s3_insert: s3 inserts 1000000 onto the new page (no SIREAD locks = bug)
+# s3_commit: s3 commits (should have been aborted by SSI)
+# s2_commit: s2 aborts due to serialization failure
+permutation
+ s1_begin
+ bump_xmin
+ s2_begin
+ s3_begin
+ s1_insert
+ s2_insert_wait_at_page_split
+ s1_commit_wait_in_SetNewSxactGlobalXmin
+ wakeup_s2_then_s1(s2_insert_wait_at_page_split,s1_commit_wait_in_SetNewSxactGlobalXmin)
+ s3_insert
+ s3_commit
+ s2_commit
+ verify
--
2.43.0
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2026-06-12 01:04 Josh Curtis <jcurtis825@gmail.com>
parent: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Josh Curtis @ 2026-06-12 01:04 UTC (permalink / raw)
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>; +Cc: pgsql-hackers
Rebased the patch plus modified the test slightly so it shows duplicate IDs
were inserted in the buggy case
On Sat, Mar 28, 2026 at 6:07 AM Mihail Nikalayeu <mihailnikalayeu@gmail.com>
wrote:
> Hello!
>
> Combined refined test and fix into a single commit to make cfbot happy.
>
> Best regards,
> Mikhail.
>
Attachments:
v4-0001-Fix-race-condition-when-reading-PredXact-SxactGlo.patch (application/octet-stream, 12.7K) download
From 6d2aeaffa0150779ab92278fbe6752b23b9288ce Mon Sep 17 00:00:00 2001
From: Josh Curtis <jcurtis825@gmail.com>
Date: Thu, 11 Jun 2026 17:06:37 -0700
Subject: [PATCH v4] Fix race condition when reading
`PredXact->SxactGlobalXmin`
`DropAllPredicateLocksFromTable`, `PredicateLockPageSplit`, and
`CheckTableForSerializableConflictIn` all assume that
`!TransactionIdIsValid(PredXact->SxactGlobalXmin)` implies there are
no active serializable transactions.
This assumption is not true due to a race with
`SetNewSxactGlobalXmin`, which first sets `PredXact->SxactGlobalXmin`
to `InvalidTransactionId` then iterates over the active serializable
transactions to find the new xmin. If `SetNewSxactGlobalXmin` is
interrupted before setting a new xmin and other processes check
`!TransactionIdIsValid(PredXact->SxactGlobalXmin)` without taking a
lock during this time, they will incorrectly assume it is safe to
proceed as if there are no concurrent serializable transactions.
Author: Josh Curtis <jcurtis825@gmail.com>
Reviewed-by: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Discussion: https://postgr.es/m/flat/CAEkjoh2X8yL9TKrQKx6SL6gtMNuZsyH8K-%2BrE78EwdOv4oXjSA%40mail.gmail.com
---
src/backend/storage/lmgr/predicate.c | 37 +++--
src/test/modules/injection_points/Makefile | 1 +
.../expected/predicate-lock-page-split.out | 108 ++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../specs/predicate-lock-page-split.spec | 133 ++++++++++++++++++
5 files changed, 267 insertions(+), 13 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/predicate-lock-page-split.out
create mode 100644 src/test/modules/injection_points/specs/predicate-lock-page-split.spec
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 0ae85b7d5b4..b2509ac0765 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -210,6 +210,7 @@
#include "storage/shmem.h"
#include "storage/subsystems.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
#include "utils/wait_event.h"
@@ -2878,12 +2879,14 @@ DropAllPredicateLocksFromTable(Relation relation, bool transfer)
/*
* Bail out quickly if there are no serializable transactions running.
- * It's safe to check this without taking locks because the caller is
- * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
- * would matter here can be acquired while that is held.
*/
+ LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ LWLockRelease(SerializableXactHashLock);
return;
+ }
+ LWLockRelease(SerializableXactHashLock);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -3079,16 +3082,15 @@ PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
/*
* Bail out quickly if there are no serializable transactions running.
- *
- * It's safe to do this check without taking any additional locks. Even if
- * a serializable transaction starts concurrently, we know it can't take
- * any SIREAD locks on the page being split because the caller is holding
- * the associated buffer page lock. Memory reordering isn't an issue; the
- * memory barrier in the LWLock acquisition guarantees that this read
- * occurs while the buffer page lock is held.
*/
+ INJECTION_POINT("predicate-lock-page-split", NULL);
+ LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ LWLockRelease(SerializableXactHashLock);
return;
+ }
+ LWLockRelease(SerializableXactHashLock);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -3186,6 +3188,10 @@ SetNewSxactGlobalXmin(void)
PredXact->SxactGlobalXmin = InvalidTransactionId;
PredXact->SxactGlobalXminCount = 0;
+#ifdef USE_INJECTION_POINTS
+ INJECTION_POINT_CACHED("predicate-set-sxact-global-xmin-invalid", NULL);
+#endif
+
dlist_foreach(iter, &PredXact->activeList)
{
SERIALIZABLEXACT *sxact =
@@ -3299,6 +3305,9 @@ ReleasePredicateLocks(bool isCommit, bool isReadOnlySafe)
Assert(LocalPredicateLockHash == NULL);
return;
}
+#ifdef USE_INJECTION_POINTS
+ INJECTION_POINT_LOAD("predicate-set-sxact-global-xmin-invalid");
+#endif
LWLockAcquire(SerializableXactHashLock, LW_EXCLUSIVE);
@@ -4355,12 +4364,14 @@ CheckTableForSerializableConflictIn(Relation relation)
/*
* Bail out quickly if there are no serializable transactions running.
- * It's safe to check this without taking locks because the caller is
- * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
- * would matter here can be acquired while that is held.
*/
+ LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
+ {
+ LWLockRelease(SerializableXactHashLock);
return;
+ }
+ LWLockRelease(SerializableXactHashLock);
if (!SerializationNeededForWrite(relation))
return;
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index c01d2fb095c..da0ead2b727 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -14,6 +14,7 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
inplace \
+ predicate-lock-page-split \
repack \
repack_temporal \
repack_temporal_multirange \
diff --git a/src/test/modules/injection_points/expected/predicate-lock-page-split.out b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
new file mode 100644
index 00000000000..8f5613c9d8e
--- /dev/null
+++ b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
@@ -0,0 +1,108 @@
+Parsed test spec with 5 sessions
+
+starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2_then_s1 s3_insert s3_commit s2_commit verify
+injection_points_attach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s1_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+
+locked
+------
+t
+(1 row)
+
+step bump_xmin:
+ DO $$
+ BEGIN
+ PERFORM pg_current_xact_id();
+ END;
+ $$;
+
+step s2_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+
+locked
+------
+t
+(1 row)
+
+step s3_begin:
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+
+locked
+------
+t
+(1 row)
+
+step s1_insert:
+ INSERT INTO test_table
+ SELECT max(id) + 1
+ FROM test_table;
+
+step s2_insert_wait_at_page_split:
+ DO $$
+ DECLARE
+ next_id int := 0;
+ base_size bigint := pg_relation_size('test_table_id_idx');
+ BEGIN
+ LOOP
+ INSERT INTO test_table VALUES (next_id);
+ EXIT WHEN pg_relation_size('test_table_id_idx') > base_size;
+ next_id := next_id - 1;
+ END LOOP;
+ END;
+ $$;
+ <waiting ...>
+step s1_commit_wait_in_SetNewSxactGlobalXmin:
+ COMMIT;
+ <waiting ...>
+step wakeup_s2_then_s1:
+ SELECT injection_points_wakeup('predicate-lock-page-split');
+ SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
+ <waiting ...>
+step s2_insert_wait_at_page_split: <... completed>
+step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed>
+step s3_insert:
+ INSERT INTO test_table
+ SELECT max(id) + 1
+ FROM test_table;
+
+ERROR: could not serialize access due to read/write dependencies among transactions
+step wakeup_s2_then_s1: <... completed>
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s3_commit:
+ COMMIT;
+
+step s2_commit:
+ COMMIT;
+
+ERROR: could not serialize access due to read/write dependencies among transactions
+step verify:
+ SELECT id, count(*) FROM test_table GROUP BY id ORDER BY id;
+
+id|count
+--+-----
+ 1| 1
+ 2| 1
+(2 rows)
+
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 59dba1cb023..94c03283335 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -45,6 +45,7 @@ tests += {
'specs': [
'basic',
'inplace',
+ 'predicate-lock-page-split',
'repack',
'repack_temporal',
'repack_temporal_multirange',
diff --git a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
new file mode 100644
index 00000000000..fd905a37b07
--- /dev/null
+++ b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
@@ -0,0 +1,133 @@
+# Test for race condition in PredicateLockPageSplit
+#
+# When SetNewSxactGlobalXmin() temporarily sets SxactGlobalXmin to
+# InvalidTransactionId, a concurrent PredicateLockPageSplit() can see
+# the invalid value and skip transferring SIREAD locks to the new page.
+# This lets a third transaction insert onto the new page without SSI
+# detecting the rw-conflict: s1 and s3 both compute max(id) + 1 from
+# the same snapshot and insert the same id twice, which no serial
+# ordering of the transactions could produce.
+#
+setup
+{
+ CREATE EXTENSION IF NOT EXISTS injection_points;
+ DROP TABLE IF EXISTS test_table;
+
+ CREATE TABLE test_table (id int);
+ CREATE INDEX test_table_id_idx ON test_table USING btree (id);
+ INSERT INTO test_table VALUES (1);
+}
+
+teardown
+{
+ DROP TABLE IF EXISTS test_table;
+ DROP EXTENSION IF EXISTS injection_points;
+}
+
+session s0
+step bump_xmin {
+ DO $$
+ BEGIN
+ PERFORM pg_current_xact_id();
+ END;
+ $$;
+}
+step verify {
+ SELECT id, count(*) FROM test_table GROUP BY id ORDER BY id;
+}
+
+session s1
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('predicate-set-sxact-global-xmin-invalid', 'wait');
+}
+step s1_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+}
+step s1_insert {
+ INSERT INTO test_table
+ SELECT max(id) + 1
+ FROM test_table;
+}
+step s1_commit_wait_in_SetNewSxactGlobalXmin {
+ COMMIT;
+}
+
+session s2
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('predicate-lock-page-split', 'wait');
+}
+step s2_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+}
+step s2_insert_wait_at_page_split {
+ DO $$
+ DECLARE
+ next_id int := 0;
+ base_size bigint := pg_relation_size('test_table_id_idx');
+ BEGIN
+ LOOP
+ INSERT INTO test_table VALUES (next_id);
+ EXIT WHEN pg_relation_size('test_table_id_idx') > base_size;
+ next_id := next_id - 1;
+ END LOOP;
+ END;
+ $$;
+}
+step s2_commit {
+ COMMIT;
+}
+
+session s3
+step s3_begin {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+ SELECT max(id) > 0 AS locked FROM test_table;
+}
+step s3_insert {
+ INSERT INTO test_table
+ SELECT max(id) + 1
+ FROM test_table;
+}
+step s3_commit {
+ COMMIT;
+}
+
+session s4
+step wakeup_s2_then_s1 {
+ SELECT injection_points_wakeup('predicate-lock-page-split');
+ SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
+}
+
+# s1_begin: s1 reads from the table, establishing SIREAD locks on the index
+# bump_xmin: advance xmin so s2/s3 get a higher xmin than s1
+# s2_begin, s3_begin: s2 and s3 read from the table (same snapshot as s1)
+#
+# s1_insert: s1 inserts max(id)+1 = 2
+# s2_insert_wait_at_page_split: s2 inserts descending values until a real
+# btree page split happens, then waits in PredicateLockPageSplit before
+# checking SxactGlobalXmin. The values must be descending so that the
+# split moves ids 1 and 2 to the new page
+# s1_commit_wait_in_SetNewSxactGlobalXmin: after s2 is already waiting,
+# s1 commits and waits after SetNewSxactGlobalXmin sets SxactGlobalXmin
+# to InvalidTransactionId
+# wakeup_s2_then_s1: wake s2 (sees InvalidTransactionId, skips SIREAD
+# lock transfer), then wake s1
+# s3_insert: s3 inserts max(id)+1 = 2, computed from its snapshot
+# s3_commit: s3 commits (should have been aborted by SSI)
+# s2_commit: s2 aborts due to serialization failure
+permutation
+ s1_begin
+ bump_xmin
+ s2_begin
+ s3_begin
+ s1_insert
+ s2_insert_wait_at_page_split
+ s1_commit_wait_in_SetNewSxactGlobalXmin
+ wakeup_s2_then_s1(s2_insert_wait_at_page_split,s1_commit_wait_in_SetNewSxactGlobalXmin)
+ s3_insert
+ s3_commit
+ s2_commit
+ verify
--
2.52.0
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2026-08-26 18:28 Andrey Borodin <x4mmm@yandex-team.ru>
parent: Josh Curtis <jcurtis825@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Andrey Borodin @ 2026-08-26 18:28 UTC (permalink / raw)
To: Josh Curtis <jcurtis825@gmail.com>; +Cc: Mihail Nikalayeu <mihailnikalayeu@gmail.com>; pgsql-hackers
> On 12 Jun 2026, at 06:04, Josh Curtis <jcurtis825@gmail.com> wrote:
>
> <v4-0001-Fix-race-condition-when-reading-PredXact-SxactGlo.patch>
Hi Josh,
I suspect a race in the test:
+step s2_insert_wait_at_page_split: <... completed>
+step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed>
Adding an explicit dependency can make the output stable:
s2_insert_wait_at_page_split(s1_commit_wait_in_SetNewSxactGlobalXmin)
I don't think we should acquire SerializableXactHashLock in shared mode
on every page split to fix this. The race exists because
SetNewSxactGlobalXmin() uses the shared fields as local variables and
publishes a transient InvalidTransactionId while recalculating them.
Could it instead calculate the new xmin and count in local variables,
then publish them after scanning the active list? Readers that use both
fields already hold SerializableXactHashLock. Lockless readers inspect
only SxactGlobalXmin and may safely see the previous valid value. That
can only cause unnecessary predicate-lock work. Publishing
SxactGlobalXmin last would preserve that property and the existing fast
path when no serializable transactions are running.
That seems both simpler and better for the page-split path. I think the
patch should fix the publication side instead of adding the three reader
locks. WDYT?
Thank you!
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix race condition in SSI when reading PredXact->SxactGlobalXmin
@ 2026-09-22 18:56 Manu <manuelreyesbravo@gmail.com>
parent: Andrey Borodin <x4mmm@yandex-team.ru>
0 siblings, 0 replies; 11+ messages in thread
From: Manu @ 2026-09-22 18:56 UTC (permalink / raw)
To: Josh Curtis <jcurtis825@gmail.com>; +Cc: Andrey Borodin <x4mmm@yandex-team.ru>; Mihail Nikalayeu <mihailnikalayeu@gmail.com>; pgsql-hackers@lists.postgresql.org
Hi Josh, Andrey,
Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> I suspect a race in the test:
>
> +step s2_insert_wait_at_page_split: <... completed>
> +step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed>
> Could it instead calculate the new xmin and count in local variables,
> then publish them after scanning the active list?
I tried both, on master e8a3ee5b197 with --enable-cassert and
--enable-injection-points. v4 needs a rebase, but only in the
injection_points Makefile and meson.build lists.
The test. Run 50 times against one cluster (the attached
run_spec_n.sh), v4's spec gives the other completion order in 50 of 50
runs with v4 applied. And it doesn't catch the bug: with v4's test and
injection points but the readers as on master, it showed id 2 twice in 1
run of 50, the first one on a fresh cluster. Under make check in
injection_points it fails there only on the completion order; verify
has no duplicate. wakeup_s2_then_s1 wakes both sessions at once, and
the window only exists while s1 is still inside
SetNewSxactGlobalXmin(), so most of the time s1 is done before s2 reads
SxactGlobalXmin.
A second injection point right after the check in
PredicateLockPageSplit() makes it deterministic: wake s2 alone, so it
checks while s1 is still inside SetNewSxactGlobalXmin() and stops right
after the check, then wake s1, then s2 again, with markers for both.
If s2 saw InvalidTransactionId it returned early and never gets to the
second point.
duplicate id distinct outputs
readers as on master 13 of 13 1
Andrey's approach 0 of 30 1
v4 (reader locks) 0 of 15 2
With v4, s2 checks under SerializableXactHashLock, which s1 holds in
exclusive mode while it waits inside SetNewSxactGlobalXmin(), so after
the first wakeup s2 waits on an LWLock, which isolationtester doesn't
see, and the output depends on whether it looks at s2 before or after
that. In an earlier round one run of eleven stopped right after
wakeup_s2 until the 60 s timeout; in the 15 above none did. (Without
the fix, one run in fourteen hung the same way, which is why only the
13 before it are counted.) So the reader locks also keep the test from
being deterministic.
Andrey's approach. The attached diff, on top of v4, does it:
SetNewSxactGlobalXmin() computes the xmin and the count in local
variables and publishes them after the scan, SxactGlobalXmin last; the
three readers go back to the lockless check, with the comments that
explained why it is safe put back plus a sentence on why it is safe
again; and the spec is the deterministic one above. With it,
injection_points (make check, three times in a row), src/test/isolation
(133) and make check (239) pass, and pgindent leaves it unchanged.
I also tried to measure the reader locks, with the attached
page_split_bench.sh: 50-row inserts of random keys into a btree, so
page splits are frequent, 8 clients, 15 s, 7 runs round-robin between
release builds, median [min - max] TPS:
no serializable xact inside SERIALIZABLE
master 27859 [18240-30818] 27643 [23660-29449]
v4 27878 [22204-31266] 27497 [24554-28122]
Andrey's approach 28570 [18464-32231] 27033 [25626-28803]
So I could not measure the cost of the locks here: the differences are
smaller than the spread between runs on this machine. That's not an
argument for the locks, just one I can't make against them.
Josh, feel free to take any of it into a v5; I'm happy to review it.
Regards,
Manu
Attachments:
nocfbot-v4-publish-xmin-after-scan.diff.txt (text/plain, 9.9K) download
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index b2509ac0765..82a9fcdd9f5 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -2879,14 +2879,15 @@ DropAllPredicateLocksFromTable(Relation relation, bool transfer)
/*
* Bail out quickly if there are no serializable transactions running.
+ * It's safe to check this without taking locks because the caller is
+ * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
+ * would matter here can be acquired while that is held.
+ * SetNewSxactGlobalXmin() publishes a new value only after scanning the
+ * active list, so InvalidTransactionId is never seen here while
+ * serializable transactions are still active.
*/
- LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
- {
- LWLockRelease(SerializableXactHashLock);
return;
- }
- LWLockRelease(SerializableXactHashLock);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -3082,15 +3083,21 @@ PredicateLockPageSplit(Relation relation, BlockNumber oldblkno,
/*
* Bail out quickly if there are no serializable transactions running.
+ *
+ * It's safe to do this check without taking any additional locks. Even if
+ * a serializable transaction starts concurrently, we know it can't take
+ * any SIREAD locks on the page being split because the caller is holding
+ * the associated buffer page lock. Memory reordering isn't an issue; the
+ * memory barrier in the LWLock acquisition guarantees that this read
+ * occurs while the buffer page lock is held. SetNewSxactGlobalXmin()
+ * publishes a new value only after scanning the active list, so
+ * InvalidTransactionId is never seen here while serializable transactions
+ * are still active.
*/
INJECTION_POINT("predicate-lock-page-split", NULL);
- LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
- {
- LWLockRelease(SerializableXactHashLock);
return;
- }
- LWLockRelease(SerializableXactHashLock);
+ INJECTION_POINT("predicate-lock-page-split-after-check", NULL);
if (!PredicateLockingNeededForRelation(relation))
return;
@@ -3182,12 +3189,17 @@ static void
SetNewSxactGlobalXmin(void)
{
dlist_iter iter;
+ TransactionId xmin = InvalidTransactionId;
+ int xmincount = 0;
Assert(LWLockHeldByMe(SerializableXactHashLock));
- PredXact->SxactGlobalXmin = InvalidTransactionId;
- PredXact->SxactGlobalXminCount = 0;
-
+ /*
+ * Compute the new values in local variables, and publish them only at the
+ * end: some callers read SxactGlobalXmin without holding
+ * SerializableXactHashLock, and must not see a transient
+ * InvalidTransactionId while serializable transactions are still active.
+ */
#ifdef USE_INJECTION_POINTS
INJECTION_POINT_CACHED("predicate-set-sxact-global-xmin-invalid", NULL);
#endif
@@ -3202,20 +3214,21 @@ SetNewSxactGlobalXmin(void)
&& sxact != OldCommittedSxact)
{
Assert(sxact->xmin != InvalidTransactionId);
- if (!TransactionIdIsValid(PredXact->SxactGlobalXmin)
- || TransactionIdPrecedes(sxact->xmin,
- PredXact->SxactGlobalXmin))
+ if (!TransactionIdIsValid(xmin)
+ || TransactionIdPrecedes(sxact->xmin, xmin))
{
- PredXact->SxactGlobalXmin = sxact->xmin;
- PredXact->SxactGlobalXminCount = 1;
+ xmin = sxact->xmin;
+ xmincount = 1;
}
- else if (TransactionIdEquals(sxact->xmin,
- PredXact->SxactGlobalXmin))
- PredXact->SxactGlobalXminCount++;
+ else if (TransactionIdEquals(sxact->xmin, xmin))
+ xmincount++;
}
}
- SerialSetActiveSerXmin(PredXact->SxactGlobalXmin);
+ PredXact->SxactGlobalXminCount = xmincount;
+ PredXact->SxactGlobalXmin = xmin;
+
+ SerialSetActiveSerXmin(xmin);
}
/*
@@ -4364,14 +4377,15 @@ CheckTableForSerializableConflictIn(Relation relation)
/*
* Bail out quickly if there are no serializable transactions running.
+ * It's safe to check this without taking locks because the caller is
+ * holding an ACCESS EXCLUSIVE lock on the relation. No new locks which
+ * would matter here can be acquired while that is held.
+ * SetNewSxactGlobalXmin() publishes a new value only after scanning the
+ * active list, so InvalidTransactionId is never seen here while
+ * serializable transactions are still active.
*/
- LWLockAcquire(SerializableXactHashLock, LW_SHARED);
if (!TransactionIdIsValid(PredXact->SxactGlobalXmin))
- {
- LWLockRelease(SerializableXactHashLock);
return;
- }
- LWLockRelease(SerializableXactHashLock);
if (!SerializationNeededForWrite(relation))
return;
diff --git a/src/test/modules/injection_points/expected/predicate-lock-page-split.out b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
index 8f5613c9d8e..d80353abf5e 100644
--- a/src/test/modules/injection_points/expected/predicate-lock-page-split.out
+++ b/src/test/modules/injection_points/expected/predicate-lock-page-split.out
@@ -1,6 +1,6 @@
Parsed test spec with 5 sessions
-starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2_then_s1 s3_insert s3_commit s2_commit verify
+starting permutation: s1_begin bump_xmin s2_begin s3_begin s1_insert s2_insert_wait_at_page_split s1_commit_wait_in_SetNewSxactGlobalXmin wakeup_s2 wakeup_s1 wakeup_s2_after_check s3_insert s3_commit s2_commit verify
injection_points_attach
-----------------------
@@ -67,24 +67,35 @@ step s2_insert_wait_at_page_split:
step s1_commit_wait_in_SetNewSxactGlobalXmin:
COMMIT;
<waiting ...>
-step wakeup_s2_then_s1:
+step wakeup_s2:
SELECT injection_points_wakeup('predicate-lock-page-split');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step wakeup_s1:
SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
<waiting ...>
-step s2_insert_wait_at_page_split: <... completed>
step s1_commit_wait_in_SetNewSxactGlobalXmin: <... completed>
+step wakeup_s1: <... completed>
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step wakeup_s2_after_check:
+ SELECT injection_points_wakeup('predicate-lock-page-split-after-check');
+ <waiting ...>
step s3_insert:
INSERT INTO test_table
SELECT max(id) + 1
FROM test_table;
ERROR: could not serialize access due to read/write dependencies among transactions
-step wakeup_s2_then_s1: <... completed>
-injection_points_wakeup
------------------------
-
-(1 row)
-
+step s2_insert_wait_at_page_split: <... completed>
+step wakeup_s2_after_check: <... completed>
injection_points_wakeup
-----------------------
diff --git a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
index fd905a37b07..8ed3f0f4223 100644
--- a/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
+++ b/src/test/modules/injection_points/specs/predicate-lock-page-split.spec
@@ -1,4 +1,6 @@
-# Test for race condition in PredicateLockPageSplit
+# Test for race condition in PredicateLockPageSplit (deterministic variant:
+# s2 decides whether to transfer the SIREAD locks while s1 is still inside
+# SetNewSxactGlobalXmin(), and stops right after that decision)
#
# When SetNewSxactGlobalXmin() temporarily sets SxactGlobalXmin to
# InvalidTransactionId, a concurrent PredicateLockPageSplit() can see
@@ -58,6 +60,7 @@ session s2
setup {
SELECT injection_points_set_local();
SELECT injection_points_attach('predicate-lock-page-split', 'wait');
+ SELECT injection_points_attach('predicate-lock-page-split-after-check', 'wait');
}
step s2_begin {
BEGIN ISOLATION LEVEL SERIALIZABLE;
@@ -96,28 +99,21 @@ step s3_commit {
}
session s4
-step wakeup_s2_then_s1 {
+step wakeup_s2 {
SELECT injection_points_wakeup('predicate-lock-page-split');
+}
+step wakeup_s2_after_check {
+ SELECT injection_points_wakeup('predicate-lock-page-split-after-check');
+}
+step wakeup_s1 {
SELECT injection_points_wakeup('predicate-set-sxact-global-xmin-invalid');
}
-# s1_begin: s1 reads from the table, establishing SIREAD locks on the index
-# bump_xmin: advance xmin so s2/s3 get a higher xmin than s1
-# s2_begin, s3_begin: s2 and s3 read from the table (same snapshot as s1)
-#
-# s1_insert: s1 inserts max(id)+1 = 2
-# s2_insert_wait_at_page_split: s2 inserts descending values until a real
-# btree page split happens, then waits in PredicateLockPageSplit before
-# checking SxactGlobalXmin. The values must be descending so that the
-# split moves ids 1 and 2 to the new page
-# s1_commit_wait_in_SetNewSxactGlobalXmin: after s2 is already waiting,
-# s1 commits and waits after SetNewSxactGlobalXmin sets SxactGlobalXmin
-# to InvalidTransactionId
-# wakeup_s2_then_s1: wake s2 (sees InvalidTransactionId, skips SIREAD
-# lock transfer), then wake s1
-# s3_insert: s3 inserts max(id)+1 = 2, computed from its snapshot
-# s3_commit: s3 commits (should have been aborted by SSI)
-# s2_commit: s2 aborts due to serialization failure
+# s1 waits inside SetNewSxactGlobalXmin(). s2 is woken alone: it reads
+# SxactGlobalXmin while s1 is still there and, if it sees a valid value, stops
+# right after the check (a lock-free check never waits on s1). Then s1 is
+# woken, and then s2. If s2 saw InvalidTransactionId, it skipped the SIREAD
+# lock transfer and never reaches the second point: the id is inserted twice.
permutation
s1_begin
bump_xmin
@@ -126,7 +122,9 @@ permutation
s1_insert
s2_insert_wait_at_page_split
s1_commit_wait_in_SetNewSxactGlobalXmin
- wakeup_s2_then_s1(s2_insert_wait_at_page_split,s1_commit_wait_in_SetNewSxactGlobalXmin)
+ wakeup_s2
+ wakeup_s1(s1_commit_wait_in_SetNewSxactGlobalXmin)
+ wakeup_s2_after_check(s2_insert_wait_at_page_split)
s3_insert
s3_commit
s2_commit
run_spec_n.sh.txt (text/plain, 1018B) download
#!/bin/bash
# usage: run_spec_n.sh <install prefix> <isolationtester> <spec file> <runs> <port>
#
# Runs one isolation spec <runs> times against a single throwaway cluster
# and counts the distinct outputs, and the runs where verify shows id 2
# twice. Each run gets 60 seconds; a run that hangs leaves backends waiting
# on injection points, so the runs after it are not meaningful.
P=$1; IT=$2; SPEC=$3; RUNS=$4; PORT=$5
D=/tmp/specn_$PORT
rm -rf $D; mkdir -p $D/out
$P/bin/initdb -D $D/data -U postgres --no-sync >/dev/null 2>&1
$P/bin/pg_ctl -D $D/data -o "-p $PORT -k /tmp" -l $D/log -w start >/dev/null
trap '$P/bin/pg_ctl -D $D/data -m immediate -w stop >/dev/null 2>&1; rm -rf $D' EXIT
for r in $(seq 1 $RUNS); do
timeout 60 $IT "host=/tmp port=$PORT dbname=postgres user=postgres" < $SPEC > $D/out/$r 2>&1 ||
echo "run $r: exit $? (124 = timed out)"
done
echo "distinct outputs:"
md5sum $D/out/* | awk '{print $1}' | sort | uniq -c
echo "runs with id 2 twice: $(grep -lE '^ *2\| *2$' $D/out/* | wc -l)"
page_split_bench.sh.txt (text/plain, 2.3K) download
#!/bin/bash
# usage: page_split_bench.sh <clients> <seconds> <runs> <prefix> [<prefix> ...]
#
# Inserts of random keys into a btree (many page splits), 50 rows per
# transaction, with pgbench:
# A: read committed, no serializable transaction around
# B: the same inside BEGIN ISOLATION LEVEL SERIALIZABLE (--max-tries 50)
# One cluster per prefix (release builds), run round-robin, median TPS.
# The clusters live in $WORKDIR (default ./psbench, on disk): millions of
# inserted rows and their WAL would fill a tmpfs /tmp.
C=$1; T=$2; R=$3; shift 3
W=${WORKDIR:-$PWD/psbench}; rm -rf $W; mkdir -p $W
cat > $W/A.sql <<'EOF'
INSERT INTO t (k) SELECT (random() * 2000000000)::int FROM generate_series(1, 50);
EOF
cat > $W/B.sql <<'EOF'
BEGIN ISOLATION LEVEL SERIALIZABLE;
INSERT INTO t (k) SELECT (random() * 2000000000)::int FROM generate_series(1, 50);
COMMIT;
EOF
i=0
for P in "$@"; do
i=$((i + 1)); port=$((56500 + i))
$P/bin/initdb -D $W/d$i -U postgres --no-sync >/dev/null 2>&1
cat >> $W/d$i/postgresql.conf <<EOF
shared_buffers = 1GB
fsync = off
synchronous_commit = off
full_page_writes = off
autovacuum = off
EOF
$P/bin/pg_ctl -D $W/d$i -o "-p $port -k /tmp" -l $W/log$i -w start >/dev/null
done
trap 'i=0; for P in "$@"; do i=$((i + 1)); $P/bin/pg_ctl -D $W/d$i -m immediate -w stop >/dev/null 2>&1; done; rm -rf $W' EXIT
for w in A B; do
for r in $(seq 1 $R); do
i=0
for P in "$@"; do
i=$((i + 1)); port=$((56500 + i))
$P/bin/psql -X -q -h /tmp -p $port -U postgres postgres \
-c "DROP TABLE IF EXISTS t" -c "CREATE TABLE t (k int)" -c "CREATE INDEX ON t (k)" -c "CHECKPOINT"
$P/bin/pgbench -n -h /tmp -p $port -U postgres -c $C -j $C -T $T --max-tries=50 \
-f $W/$w.sql postgres > $W/pgbench.out 2>&1
tps=$(grep -oE 'tps = [0-9.]+' $W/pgbench.out | grep -oE '[0-9.]+')
[ -n "$tps" ] || { echo "pgbench failed ($w, $P):" >&2; tail -3 $W/pgbench.out >&2; }
echo "$w $i $tps" | tee -a $W/raw
done
done
done
echo "TPS ($C clients, $T s, $R runs): median [min - max]"
for w in A B; do
i=0
for P in "$@"; do
i=$((i + 1))
awk -v w=$w -v n=$i '$1==w && $2==n && $3 != "" {print $3}' $W/raw | sort -n |
awk -v w=$w -v p=$P '{a[NR]=$1} END {printf " %s %-40s %8.0f [%.0f - %.0f] (%d runs)\n", w, p, a[int((NR+1)/2)], a[1], a[NR], NR}'
done
done
end of thread, other threads:[~2026-09-22 18:56 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-09-04 22:09 Fix race condition in SSI when reading PredXact->SxactGlobalXmin Josh Curtis <jcurtis825@gmail.com>
2025-10-07 08:19 ` Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2025-10-20 21:30 ` Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2025-10-21 00:55 ` Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2025-10-22 03:07 ` Josh Curtis <jcurtis825@gmail.com>
2026-02-21 21:15 ` Josh Curtis <jcurtis825@gmail.com>
2026-03-08 19:21 ` Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2026-03-28 13:07 ` Mihail Nikalayeu <mihailnikalayeu@gmail.com>
2026-06-12 01:04 ` Josh Curtis <jcurtis825@gmail.com>
2026-08-26 18:28 ` Andrey Borodin <x4mmm@yandex-team.ru>
2026-09-22 18:56 ` Manu <manuelreyesbravo@gmail.com>
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