Re: POC: Unlocked path for GetSnapshotDataReuse

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