POC: Unlocked path for GetSnapshotDataReuse

Hi,

On Mon, Aug 03, 2026 at 01:17:12PM +0200, Matthias van de Meent wrote:

Hi,

GetSnapshotData() takes the ProcArrayLock in read-only mode for several reasons:

1. To acquire new snapshot data;
2. To make sure that no active transaction commits before we install
the new snapshot data's xmin, if we didn't already have an xmin
installed; and
3. To validate that the snapshot it currently holds hasn't been
invalidated by a newly committed transaction, by comparing
snapshot->transXactCompletionCount against
TransamVariables->xactCompletionCount.

I noticed [0] that the reuse path could be implemented as just an
atomic read (thus avoiding touching the cache line that backs
ProcArrayLock) if these conditions hold:

1. We have a cached snapshot that we'd like to reuse;
2. Our backend already has its MyProc->xmin installed; and
3. The output of an atomic read of
TransamVariables->xactCompletionCount indicates its value hasn't
changed since the to-be-reused snapshot was taken.

Thanks for the patch! I agree that avoiding ProcArrayLock here makes sense when
the cached snapshot can be reused and MyProc->xmin is already installed.

Actually updating the snapshot contents would still require taking the
lock, and likewise would installing MyProc->xmin, but for some
workloads this'll probably save a lot of RW traffic on the
ProcArrayLock cache line.

Data collected from running the test suite with some instrumentation
(in 0002-nocfbot.patch) indicates ~50% of GetSnapshotData() calls
benefit from this unlocked GetSnapshotDataReuse optimization: 1174391
of 2288104 calls to GetSnapshotData used the new unlocked path, with
866907 (37%) not taking the path because it would need to install an
xmin, and 246134 (11%) failing on the xactCompletionCount check and
needing to update the snapshot, whilst the remaining 672 (0.03%) of
the sessions didn't have a cached snapshot to reuse.

I've attached a patch that applies this optimization. The patch is
quite a bit larger larger than I'd hoped, because adding
port/atomics.h to access/transam.h causes frontend compilation errors.
This effectively required me to move TransamVariables into a different
header, this case a new varsup.h, which we then need to included in
many sources, which increases the size of the patch.

I wonder if it wouldn't make more sense to move only xactCompletionCount out of
TransamVariables, rather than moving the entire TransamVariablesData definition?

xactCompletionCount is only used by procarray.c. Furthermore, the existing comment
says that the grouping in TransamVariables is largely historical.

That said, transaction completion normally also updates latestCompletedXid,
so separating them would make writers touch two cache lines: it might be worth
comparing the current placement in TransamVariables with this proposal.

+  pg_memory_barrier();
+
+  completions = pg_atomic_read_u64(&TransamVariables->xactCompletionCount);

IIUC, the barriers order the surrounding memory accesses, but they do not guarantee
that pg_atomic_read_u64() does not return an old value.

I think this needs pg_atomic_read_membarrier_u64() instead.

Since pg_atomic_read_membarrier_u64() may be more expensive it would be worth
benchmarking though.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On 2026-08-03 13:17:12 +0200, Matthias van de Meent wrote:

Hi,
GetSnapshotData() takes the ProcArrayLock in read-only mode for several reasons:

1. To acquire new snapshot data;
2. To make sure that no active transaction commits before we install
the new snapshot data's xmin, if we didn't already have an xmin
installed; and
3. To validate that the snapshot it currently holds hasn't been
invalidated by a newly committed transaction, by comparing
snapshot->transXactCompletionCount against
TransamVariables->xactCompletionCount.

I noticed [0] that the reuse path could be implemented as just an
atomic read (thus avoiding touching the cache line that backs
ProcArrayLock) if these conditions hold:

[0] after implementing this, I looked at commit logs for the Reuse
path, it looks like the original xactCompletionCount commit 623a9ba79b
already hinted at this optimization in its message as being a likely
possible further optimization. However, nobody seems to have since
gotten to actually implementing it.

That was indeed always the goal. When I was working on that whole thing the
patch initially did that, but I just had too many nagging doubts that it was
entirely safe (and had a hard time roping in others to think about it), and
decided that the overall wins were big enough to forgo that optimization for
now.

It's a pretty nice win in a bunch of workloads though, so we really ought to
do that.

I think it might be possible to even do the ->xmin assignment without a lock,
but that might take more thought. That'd make it *much* more broadly
applicable.

I've attached a patch that applies this optimization. The patch is
quite a bit larger larger than I'd hoped, because adding
port/atomics.h to access/transam.h causes frontend compilation errors.
This effectively required me to move TransamVariables into a different
header, this case a new varsup.h, which we then need to included in
many sources, which increases the size of the patch.

I'd encourage doing that in a separate commit, makes it easier to review the
actually interesting bits.

@@ -765,7 +766,7 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
MaintainLatestCompletedXid(latestXid);

/* Same with xactCompletionCount  */
-	TransamVariables->xactCompletionCount++;
+	pg_atomic_add_fetch_u64(&TransamVariables->xactCompletionCount, 1);
}

...

@@ -934,7 +935,7 @@ ProcArrayClearTransaction(PGPROC *proc)
* otherwise could end up reusing the snapshot later. Which would be bad,
* because it might not count the prepared transaction as running.
*/
-	TransamVariables->xactCompletionCount++;
+	pg_atomic_add_fetch_u64(&TransamVariables->xactCompletionCount, 1);

/* Clear the subtransaction-XID cache too */
Assert(ProcGlobal->subxidStates[pgxactoff].count == proc->subxidStatus.count &&

Doing another atomic operation while holding ProcArrayLock exclusively will
actually *increase* pressure on ProcArrayLock. Given that there can't be two
backends doing xactCompletionCount++ at the same time, due to holding
ProcArrayLock exclusively, why does these need to be an atomic increments?

+static inline bool
+GetSnapshotReuseUnlocked(Snapshot snapshot, bool *try_reuse)
+{
+	uint64		completions;
+
+	if (!TransactionIdIsValid(MyProc->xmin))
+	{
+		*try_reuse = true;
+		return false;
+	}
+
+	if (unlikely(snapshot->snapXactCompletionCount == 0))
+	{
+		*try_reuse = false;
+		return false;
+	}
+
+	pg_memory_barrier();

You really should document what precisely that barrier pairs with.

+	completions = pg_atomic_read_u64(&TransamVariables->xactCompletionCount);
+
+	if (snapshot->snapXactCompletionCount != completions)
+	{
+		*try_reuse = false;
+		return false;
+	}
+
+	pg_memory_barrier();

What does this barrier do?

Greetings,

Andres Freund

Hi,

On 2026-10-09 05:57:52 +0000, Bertrand Drouvot wrote:

+  pg_memory_barrier();
+
+  completions = pg_atomic_read_u64(&TransamVariables->xactCompletionCount);

IIUC, the barriers order the surrounding memory accesses, but they do not guarantee
that pg_atomic_read_u64() does not return an old value.

I do not believe that that is true. Except in the sense of allowing an update
to the atomic variable having happened between pg_memory_barrier() and the
pg_atomic_read_u64(), but that's something that is indistinguishable from the
increment happening happeng just after pg_atomic_read_membarrier_u64().

Greetings,

Andres Freund

Andres, Bertrand, thanks for the reviews!

On Fri, 9 Oct 2026 at 14:49, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2026-08-03 13:17:12 +0200, Matthias van de Meent wrote:

Hi,

I think it might be possible to even do the ->xmin assignment without a lock,
but that might take more thought. That'd make it *much* more broadly
applicable.

I think I have a theory of operations for this:

1. Currently, we allow setting xmin without a write-lock.
Instead, it's allowed even whilst holding LW_SHARED. This means
there are (should be) no conflicting LW_SHARED readers that may find
problem with us setting the field whilst they're examining our PGPROC
entry.
2. There are no other backends that examine or update our xmin under
LW_EXCLUSIVE.
Only prepared transactions process PGPROC entries that are not
their own, and those are never proc entries owned by other backends.

So, updating the xmin itself is OK.

Then we need to make sure we don't use the snapshot if a transaction
commits after we checked the completion count, but before we
completely updated our xmin. So, once we've inserted the old xmin, we
have to check that the xmin is still valid with the usual completion
count test. If that changed, we have to get a new snapshot -- if not,
then we've just saved ourselves a lock acquisition.

I've tried my hand at implementing that, and it's included as 0003.

Note: This does have one important effect, in that it further
normalizes vacuum horizons retreating, rather than them moving forward
monotonically.
It's well-known that some systems in PostgreSQL can cause vacuum
horizons to move backward (both for short and for longer periods), and
this will be one more of those systems.

This effectively required me to move TransamVariables into a different
header, this case a new varsup.h, which we then need to included in
many sources, which increases the size of the patch.

I'd encourage doing that in a separate commit, makes it easier to review the
actually interesting bits.

As attached, 0001.

@@ -765,7 +766,7 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
MaintainLatestCompletedXid(latestXid);

/* Same with xactCompletionCount  */
-     TransamVariables->xactCompletionCount++;
+     pg_atomic_add_fetch_u64(&TransamVariables->xactCompletionCount, 1);
}

...

@@ -934,7 +935,7 @@ ProcArrayClearTransaction(PGPROC *proc)
* otherwise could end up reusing the snapshot later. Which would be bad,
* because it might not count the prepared transaction as running.
*/
-     TransamVariables->xactCompletionCount++;
+     pg_atomic_add_fetch_u64(&TransamVariables->xactCompletionCount, 1);

/* Clear the subtransaction-XID cache too */
Assert(ProcGlobal->subxidStates[pgxactoff].count == proc->subxidStatus.count &&

Doing another atomic operation while holding ProcArrayLock exclusively will
actually *increase* pressure on ProcArrayLock. Given that there can't be two
backends doing xactCompletionCount++ at the same time, due to holding
ProcArrayLock exclusively, why does these need to be an atomic increments?

This was the simplest way to update the value whilst making sure we
don't have torn reads/writes. In the attached patch 0002 I've
replaced it with a helper that should retain untorn reads/writes in
the attached version.

-----

Attached is v2, which does the following things different from v1:

0001: Contains refactoring to move TransamVariables into its own
header, so that it can support pg_atomic types without breaking
countless FRONTEND libraries

0002: Implements the simple lockless reuse optimization of v1, with
fallback to locked reuse tests if !TransactionIdIsValid(MyProc->xmin).
The manual membarriers of v1 have been removed, they didn't add any
additional safety.

0003: Implements full lockless snapshot reuse, even when
!TransactionIdIsValid(MyProc->xmin).
This does use pg_atomic_read_membarrier for the reads of
xactCompletionCount, so that the reads are not reordered relative to
the other memory reads in the GetSnapshotDataReuse function, but the
cost of memory-barrier reads should be lower than the cost of the
writes when we acquire the LwLock.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

Hi,

On Fri, Oct 09, 2026 at 11:22:38PM +0200, Matthias van de Meent wrote:

On Fri, 9 Oct 2026 at 14:49, Andres Freund <andres@anarazel.de> wrote:

I think it might be possible to even do the ->xmin assignment without a lock,
but that might take more thought. That'd make it *much* more broadly
applicable.

I think I have a theory of operations for this:

1. Currently, we allow setting xmin without a write-lock.
Instead, it's allowed even whilst holding LW_SHARED. This means
there are (should be) no conflicting LW_SHARED readers that may find
problem with us setting the field whilst they're examining our PGPROC
entry.
2. There are no other backends that examine or update our xmin under
LW_EXCLUSIVE.
Only prepared transactions process PGPROC entries that are not
their own, and those are never proc entries owned by other backends.

So, updating the xmin itself is OK.

Then we need to make sure we don't use the snapshot if a transaction
commits after we checked the completion count, but before we
completely updated our xmin. So, once we've inserted the old xmin, we
have to check that the xmin is still valid with the usual completion
count test. If that changed, we have to get a new snapshot -- if not,
then we've just saved ourselves a lock acquisition.

Yeah, this approach makes sense to me. After the first counter check, the unchanged
completion count means that the cached snapshot is still valid, so its xmin is
safe to publish. MyProc->xmin protects the data needed by the snapshot afterwards,
and the second counter check detects a completion between the two checks.

0003: Implements full lockless snapshot reuse, even when
!TransactionIdIsValid(MyProc->xmin).
This does use pg_atomic_read_membarrier for the reads of
xactCompletionCount, so that the reads are not reordered relative to
the other memory reads in the GetSnapshotDataReuse function, but the
cost of memory-barrier reads should be lower than the cost of the
writes when we acquire the LwLock.

So we have:

+       curXactCompletionCount = pg_atomic_read_membarrier_u64(
+               &TransamVariables->xactCompletionCount
+       );

and

+xmin_installed:
+       curXactCompletionCount = pg_atomic_read_membarrier_u64(
+               &TransamVariables->xactCompletionCount
+       )

while:

+static inline void
+IncrementXactCompletionCount(void)
+{
+       uint64  xcc;
+
+       Assert(LWLockHeldByMeInMode(ProcArrayLock, LW_EXCLUSIVE));
+
+       xcc = pg_atomic_read_u64(&TransamVariables->xactCompletionCount);
+       xcc += 1;
+       pg_atomic_write_u64(&TransamVariables->xactCompletionCount, xcc);
+}

I've 2 comments here:

=== 1

pg_atomic_read_membarrier_u64() is documented to return the current value when
updates use operations with barrier semantics, while IncrementXactCompletionCount()
uses pg_atomic_write_u64().

IIUC, this is safe because the counter update is followed by the full barrier
LWLockRelease(ProcArrayLock). I could not find another place relying on such a
pairing though: existing callers use barrier semantic writes.

Worth documenting this explicitly?

=== 2

As pg_atomic_read_membarrier_u64() is potentially more expensive than
pg_atomic_read_u64() then, what about keeping pg_atomic_read_membarrier_u64()
at xmin_installed, but using pg_atomic_read_u64() for the first read? Every
successful reuse passes through xmin_installed, so the second read is the final
validation.

But then, the first read may return a stale value, but this should be safe: a
mismatch only causes a fallback, while a match is validated by the membarrier read
at xmin_installed.

This would trade one unconditional pg_atomic_read_membarrier_u64() call for the
possibility of extra work when the relaxed read is stale. Worth benchmarking?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Fri, Oct 09, 2026 at 08:54:04AM -0400, Andres Freund wrote:

Hi,

On 2026-10-09 05:57:52 +0000, Bertrand Drouvot wrote:

+  pg_memory_barrier();
+
+  completions = pg_atomic_read_u64(&TransamVariables->xactCompletionCount);

IIUC, the barriers order the surrounding memory accesses, but they do not guarantee
that pg_atomic_read_u64() does not return an old value.

I do not believe that that is true. Except in the sense of allowing an update
to the atomic variable having happened between pg_memory_barrier() and the
pg_atomic_read_u64(), but that's something that is indistinguishable from the
increment happening happeng just after pg_atomic_read_membarrier_u64().

Yeah, I think that you're right.

BTW, this thread makes me think about your blog post [1]https://techcommunity.microsoft.com/blog/adforpostgresql/improving-postgres-connection-scalability-snapshots/1806462. I looked at the
"Opportunities for further improvements" and will give this one a try:

"
Looking further into the future, it may very well be worthwhile to maintain efficient
"running transactions with xids" data structure, instead of the current "xids of
all established connections" (commonly filled largely with invalid xids).
"

[1]: https://techcommunity.microsoft.com/blog/adforpostgresql/improving-postgres-connection-scalability-snapshots/1806462

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com