Re: Fixing a couple of buglets in how VACUUM sets visibility map bits

On Tue, Jan 10, 2023 at 2:48 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> What I actually said was that there is no reason to declare up front
> that the only circumstances under which a fix could be committed is
> when a clean repro is available. I never said that a test case has
> little or no value, and I certainly didn't assert that we definitely
> don't need a test case to proceed with a commit -- since I am not in
> the habit of presumptuously attaching conditions to such things well
> in advance.

I don't understand what distinction you're making. It seems like
hair-splitting to me. We should be able to reproduce problems like
this reliably, at least with the aid of a debugger and some
breakpoints, before we go changing the code. The risk of being wrong
is quite high because the code is subtle, and the consequences of
being wrong are potentially very bad because the code is critical to
data integrity. If the reproducer doesn't require a debugger or other
extreme contortions, then we should consider reducing it to a TAP test
that can be committed. If you agree with that, then I'm not sure what
your last email was complaining about. If you disagree, then I don't
know why.

> I have been unable to reproduce the problem, and think it's possible
> that the issue cannot be triggered in practice. Though only through
> sheer luck. Here's why that is:
>
> While pruning will remove aborted dead tuples, freezing will not
> remove the xmax of an aborted update unless the XID happens to be <
> OldestXmin. With my problem scenario, the page will be all_visible in
> prunestate, but not all_frozen -- so it dodges the relevant
> visibilitymap_set() call site.
>
> That just leaves inserts that abort, I think. An aborted insert will
> be totally undone by pruning, but that does still leave behind an
> LP_DEAD item that needs to be vacuumed in the second heap pass. This
> means that we can only set the page all-visible/all-frozen in the VM
> in the second heap pass, which also dodges the relevant
> visibilitymap_set() call site.

I guess I'm not very sure that this is sheer luck. It seems like we
could equally well suppose that the people who wrote the code
correctly understood the circumstances under which we needed to avoid
calling visibilitymap_set(), and wrote the code in a way that
accomplished that purpose. Maybe there's contrary evidence or maybe it
is actually broken somehow, but that's not currently clear to me.

For the purposes of clarifying my understanding, is this the code
you're principally worried about?

/*
* If the all-visible page is all-frozen but not marked as such yet,
* mark it as all-frozen. Note that all_frozen is only valid if
* all_visible is true, so we must check both prunestate fields.
*/
else if (all_visible_according_to_vm && prunestate.all_visible &&
prunestate.all_frozen &&
!VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer))
{
/*
* We can pass InvalidTransactionId as the cutoff XID here,
* because setting the all-frozen bit doesn't cause recovery
* conflicts.
*/
visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr,
vmbuffer, InvalidTransactionId,
VISIBILITYMAP_ALL_FROZEN);
}

Or maybe this one?

if (PageIsAllVisible(page))
{
uint8 flags = 0;
uint8 vm_status = visibilitymap_get_status(vacrel->rel,
blkno, vmbuffer);

/* Set the VM all-frozen bit to flag, if needed */
if ((vm_status & VISIBILITYMAP_ALL_VISIBLE) == 0)
flags |= VISIBILITYMAP_ALL_VISIBLE;
if ((vm_status & VISIBILITYMAP_ALL_FROZEN) == 0 && all_frozen)
flags |= VISIBILITYMAP_ALL_FROZEN;

Assert(BufferIsValid(*vmbuffer));
if (flags != 0)
visibilitymap_set(vacrel->rel, blkno, buffer, InvalidXLogRecPtr,
*vmbuffer, visibility_cutoff_xid, flags);
}

These are the only two call sites in vacuumlazy.c where I can see
there being a theoretical risk of the kind of problem that you're
describing.

--
Robert Haas
EDB: http://www.enterprisedb.com