Re: BUG #17257: (auto)vacuum hangs within lazy_scan_prune()

On Wed, 3 Nov 2021 at 17:21, Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Wed, Nov 3, 2021 at 8:46 AM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > I seem to repeatedly get backends of which the xmin is set from
> > InvalidTransactionId to some value < min(ProcGlobal->xids), which then
> > result in shared_oldest_nonremovable (and others) being less than the
> > value of their previous iteration. This leads to the infinite loop in
> > lazy_scan_prune (it stores and uses one value of
> > *_oldest_nonremovable, whereas heap_page_prune uses a more up-to-date
> > variant).
>
> > I noticed that when this happens, generally a parallel vacuum worker
> > is involved.
>
> Hmm. That is plausible. The way that VACUUM (and concurrent index
> builds) avoid being seen via the PROC_IN_VACUUM thing is pretty
> delicate. Wouldn't surprise me if the parallel VACUUM issue subtly
> broke lazy_scan_prune in the way that we see here.
>
> What about testing? Can we find a simple way of reducing this
> complicated repro to a less complicated repro with a failing
> assertion? Maybe an assertion that we get to keep after the bug is
> fixed?

I added the attached instrumentation for checking xmin validity, which
asserts what I believe are correct claims about the proc
infrastructure:

- It is always safe to set ->xmin to InvalidTransactionId: This
removes any claim that we have a snapshot anyone should worry about.
- If we have a valid ->xmin set, it is always safe to increase its value.
- Otherwise, the xmin must not lower the overall xmin of the database
it is connected to, plus some potential conditions for status flags.
  It also may not be set without first taking the ProcArrayLock:
without synchronised access to the proc array, you cannot guarantee
you can set your xmin to a globally correct value.

It worked well with the bgworker flags patch [0], until I added this
instrumentation to SnapshotResetXmin and ran the regression tests: I
stumbled upon the following issue with aborting transactions, and I
don't know what the correct solution is to solve it:

AbortTransaction (see xact.c) calls ProcArrayEndTransaction, which can
reset MyProc->xmin to InvalidTransactionId (both directly and through
ProcArrayEndTransactionInternal). So far, this is safe.

However, later in AbortTransaction we call ResourceOwnerRelease(...,
RESOURCE_RELEASE_AFTER_LOCKS...), which will clean up the snapshots
stored in its owner->snapshotarr array using UnregisterSnapshot.
Then, if UnregisterSnapshot determines that a snapshot is now not
referenced anymore, and that snapshot has no active count, then it
will call SnapshotResetXmin().
Finally, when SnapshotResetXmin() is called, the oldest still
registered snapshot in RegisteredSnapshots will be pulled and
MyProc->xmin will be set to that snapshot's xmin.

Similarly, in AbortTransaction we call AtEOXact_Inval, which calls
ProcessInvalidationMessages -> LocalExecuteInvalidationMessage ->
InvalidateCatalogSnapshot -> SnapshotResetXmin, also setting
MyProc->xmin back to a non-InvalidXid value.

Note that from a third-party observer's standpoint we've just moved
our horizons backwards, and the regression tests (correctly) fail when
assertions are enabled.

I don't know what the expected behaviour is, but I do know that this
is a violation of the expected invariant of xmin never goes backwards
(for any of the cluster, database or data level).

Kind regards,

Matthias van de Meent


[0] https://www.postgresql.org/message-id/CAD21AoDkERUJkGEuQRiyGKmVRt2duU378UgnwBpqXQjA%2BEY3Lg%40mail.gmail.com