Re: vac_truncate_clog()'s bogus check leads to bogusness

On Fri, Jun 23, 2023 at 06:41:58PM -0700, Andres Freund wrote:
> On 2023-06-21 21:50:39 -0700, Noah Misch wrote:
> > On Wed, Jun 21, 2023 at 05:46:37PM -0700, Andres Freund wrote:
> > > A related issue is that as far as I can tell the determination of what is
> > > bogus is bogus.
> > >
> > > The relevant cutoffs are determined vac_update_datfrozenxid() using:
> > >
> > > /*
> > > * Identify the latest relfrozenxid and relminmxid values that we could
> > > * validly see during the scan. These are conservative values, but it's
> > > * not really worth trying to be more exact.
> > > */
> > > lastSaneFrozenXid = ReadNextTransactionId();
> > > lastSaneMinMulti = ReadNextMultiXactId();
> > >
> > > but doing checks based on thos is bogus, because:
> > >
> > > a) a concurrent create table / truncate / vacuum can update
> > > pg_class.relfrozenxid of some relation in the current database to a newer
> > > value, after lastSaneFrozenXid already has been determined. If that
> > > happens, we skip updating pg_database.datfrozenxid.
> > >
> > > b) A concurrent vacuum in another database, ending up in vac_truncate_clog(),
> > > can compute a newer datfrozenxid. In that case the vac_truncate_clog() with
> > > the outdated lastSaneFrozenXid will not truncate the clog (and also forget
> > > to release WrapLimitsVacuumLock currently, as reported upthread) and not
> > > call SetTransactionIdLimit(). The latter is particularly bad, because that
> > > means we might not come out of "database is not accepting commands" land.
> >
> > > I guess we could just recompute the boundaries before actually believing the
> > > catalog values are bogus?
> >
> > That's how I'd do it.
>
> I was looking at doing that and got confused by the current code. Am I missing
> something, or does vac_truncate_clog() have two pretty much identical attempts
> at a safety measures?
>
> void
> vac_update_datfrozenxid(void)
> ...
> lastSaneFrozenXid = ReadNextTransactionId();
> ...
> vac_truncate_clog(newFrozenXid, newMinMulti,
> lastSaneFrozenXid, lastSaneMinMulti);
> }
> ...
> static void
> vac_truncate_clog(TransactionId frozenXID,
> MultiXactId minMulti,
> TransactionId lastSaneFrozenXid,
> MultiXactId lastSaneMinMulti)
> {
> TransactionId nextXID = ReadNextTransactionId();
> ...
> /*
> * If things are working properly, no database should have a
> * datfrozenxid or datminmxid that is "in the future". However, such
> * cases have been known to arise due to bugs in pg_upgrade. If we
> * see any entries that are "in the future", chicken out and don't do
> * anything. This ensures we won't truncate clog before those
> * databases have been scanned and cleaned up. (We will issue the
> * "already wrapped" warning if appropriate, though.)
> */
> if (TransactionIdPrecedes(lastSaneFrozenXid, datfrozenxid) ||
> MultiXactIdPrecedes(lastSaneMinMulti, datminmxid))
> bogus = true;
>
> if (TransactionIdPrecedes(nextXID, datfrozenxid))
> frozenAlreadyWrapped = true;
>
> lastSaneFrozenXid is a slightly older version of ReadNextTransactionId(),
> that's the only difference afaict.

I don't think you missed anything. nextXID and lastSaneFrozenXid are both
just caches of ReadNextTransactionId(). Each can become stale enough to make
those comparisons suggest trouble when all is fine.

> I guess this might be caused by 78db307bb23 adding the check, but using
> GetOldestXmin(NULL, true) to determine lastSaneFrozenXid. That was changed
> soon after, in 87f830e0ce03.

Yeah. The nextXID check is from 9c54cfb (2002-04), and the newer check
converged with it in 87f830e0ce03 (2014-07).

While less important, some other things look weak in these functions:

- The only non-corruption cause to reach the "don't want to let datfrozenxid
go backward" code is for GetOldestNonRemovableTransactionId(NULL) to go
backward, e.g. if a walsender starts up and advertises an xmin. One could
eliminate that cause by replacing "newFrozenXid =
GetOldestNonRemovableTransactionId(NULL)" with initialization from the first
relfrozenxid, analogous to how vac_truncate_clog() initializes.
vac_update_datfrozenxid() could then warn if the prevention code intervenes.
Perhaps, instead of preventing the go-backwards, it should apply the
go-backward change after warning? (Unlike datfrozenxid, datminmxid going
backward already implies corruption.)

- The "some databases have not been vacuumed in over 2 billion transactions"
message is false more often than not. More likely, something corrupted a
frozen ID. The message is also missing the opportunity to indicate one of
the affected databases.

- vac_truncate_clog() bogosity checks examine XIDs only, not multis.