Fix race condition in SSI when reading PredXact->SxactGlobalXmin
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t52240psql -h localhost -U postgresBuilt from patchset v9 (message #9), July 29, 2026 at 12:36 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t52240_9 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t52240_9 && git checkout t52240_9Patchset v9 (message #9) is on t52240_9
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]https://github.com/joshcurtis/ssi_race_condition_reproduction
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.patchapplication/octet-stream; name=v1-0001-Fix-race-condition-when-reading-PredXact-SxactGlo.patchDownload+15-14
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.
To prevent it being lost forever - I registered commitfest entry for it:
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.
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:
Show quoted text
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.
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:
Show quoted text
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.patchapplication/octet-stream; name=v1-0001-Add-test-to-demonstrate-bug-in-SSI.patchDownload+189-1
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.patchapplication/octet-stream; name=v2-0001-PATCH-v2-Add-test-to-demonstrate-bug-in-SSI.patchDownload+244-1
Hello!
Combined refined test and fix into a single commit to make cfbot happy.
Best regards,
Mikhail.
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:
Show quoted text
Hello!
Combined refined test and fix into a single commit to make cfbot happy.
Best regards,
Mikhail.
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.
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