Re: 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