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)
