Re: POC: Unlocked path for GetSnapshotDataReuse - Mailing list pgsql-hackers

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