Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations

On Tue, Mar 29, 2022 at 11:10 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> + diff = (int32) (vacrel->NewRelfrozenXid - vacrel->relfrozenxid);
> + Assert(diff > 0);
>
> Did you see that this crashed on windows cfbot?
>
> https://api.cirrus-ci.com/v1/artifact/task/4592929254670336/log/tmp_check/postmaster.log
> TRAP: FailedAssertion("diff > 0", File: "c:\cirrus\src\backend\access\heap\vacuumlazy.c", Line: 724, PID: 5984)

That's weird. There are very similar assertions a little earlier, that
must have *not* failed here, before the call to vac_update_relstats().
I was actually thinking of removing this assertion for that reason --
I thought that it was redundant.

Perhaps something is amiss inside vac_update_relstats(), where the
boolean flag that indicates that pg_class.relfrozenxid was advanced is
set:

if (frozenxid_updated)
*frozenxid_updated = false;
if (TransactionIdIsNormal(frozenxid) &&
pgcform->relfrozenxid != frozenxid &&
(TransactionIdPrecedes(pgcform->relfrozenxid, frozenxid) ||
TransactionIdPrecedes(ReadNextTransactionId(),
pgcform->relfrozenxid)))
{
if (frozenxid_updated)
*frozenxid_updated = true;
pgcform->relfrozenxid = frozenxid;
dirty = true;
}

Maybe the "existing relfrozenxid is in the future, silently update
relfrozenxid" part of the condition (which involves
ReadNextTransactionId()) somehow does the wrong thing here. But how?

The other assertions take into account the fact that OldestXmin can
itself "go backwards" across VACUUM operations against the same table:

Assert(!aggressive || vacrel->NewRelfrozenXid == OldestXmin ||
TransactionIdPrecedesOrEquals(FreezeLimit,
vacrel->NewRelfrozenXid));

Note the "vacrel->NewRelfrozenXid == OldestXmin", without which the
assertion will fail pretty easily when the regression tests are run.
Perhaps I need to do something like that with the other assertion as
well (or more likely just get rid of it). Will figure it out tomorrow.

--
Peter Geoghegan