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

On Wed, Jun 21, 2023 at 03:12:08PM -0700, Andres Freund wrote:
> When vac_truncate_clog() returns early
...
> we haven't released the lwlock that we acquired earlier

> Until there's some cause for the session to call LWLockReleaseAll(), the lock
> is held. Until then neither the process holding the lock, nor any other
> process, can finish vacuuming. We don't even have an assert against a
> self-deadlock with an already held lock, oddly enough.

I agree with this finding. Would you like to add the lwlock releases, or
would you like me to?

The bug has been in all released versions for 2.5 years, yet it escaped
notice. That tells us something. Bogus values have gotten rare? The
affected session tends to get lucky and call LWLockReleaseAll() soon?

On Wed, Jun 21, 2023 at 05:46:37PM -0700, Andres Freund wrote:
> On 2023-06-21 15:12:08 -0700, Andres Freund wrote:
> > Separately, I think it's quite bad that we *silently* return from
> > vac_truncate_clog() when finding a bogus xid. That's a quite severe condition,
> > we should at least tell the user about it.
>
> 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.