Re: Race conditions in logical decoding

Rui Zhao <zhaorui126(at)gmail(dot)com> wrote:

> 1. The wait belongs in SnapBuildInitialSnapshot() and nowhere else:
> SnapBuildBuildSnapshot() does not need it, and in
> SnapBuildInitialSnapshot() it can be a wait on the transaction lock.
> 0001 does that.
>
> SnapBuildInitialSnapshot() is the only place where the builder's list of
> committed transactions turns into a regular MVCC snapshot, and it is
> HeapTupleSatisfiesMVCC() on that snapshot that asks CLOG about a
> transaction between xmin and xmax. The historic snapshots that
> SnapBuildBuildSnapshot() hands to the reorder buffer never do:
> HeapTupleSatisfiesHistoricMVCC() decides the range [xmin, xmax) by the
> xip array alone and consults CLOG only below xmin, and builder->xmin is
> always the oldestRunningXid of an xl_running_xacts record, so a
> transaction below it had left the procarray, and so updated CLOG, before
> that record was written.

I initially thought that it's silly to rely on such tricky details, but not
consulting CLOG appears to be a design choice - see the header comment in
snapbuild.c.

* ........ Also, our snapshots need to be different in comparison to normal
* MVCC ones because in contrast to those we cannot fully rely on the clog and
* pg_subtrans for information about committed transactions because they might
* commit in the future from the POV of the WAL entry we're currently
* decoding. ...

And regarding snapshot's xmin, I agree that it's controlled by
xl_running_xacts WAL record and that it does not advance until the transaction
has been recorded in CLOG.

Thus I'm not opposed to the idea that it's enough to add the check to
SnapBuildInitialSnapshot().

> + if (!RecoveryInProgress())
> + {
> + RunningTransactions running;
> + int nrunning;
> +
> + running = GetRunningTransactionData();
> + nrunning = running->xcnt + running->subxcnt;
> + LWLockRelease(ProcArrayLock);
> + LWLockRelease(XidGenLock);
> +
> + for (int i = 0; i < nrunning; i++)
> + {
> + TransactionId running_xid = running->xids[i];
> +
> + if (bsearch(&running_xid, snap->xip, snap->xcnt,
> + sizeof(TransactionId), xidComparator) != NULL)
> + XactLockTableWait(running_xid, NULL, NULL, XLTW_None);
> + }
> + }

I don't understand why you check all transactions in procarray, instead of
only those in snap->xip.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com