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

Hi,

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 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.


Am I missing something?


Greetings,

Andres Freund