Re: Incorrect assumption in heap_prepare_freeze_tuple

On Sat, Oct 3, 2020 at 1:06 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2020-10-03 12:58:01 +0530, Kuntal Ghosh wrote:
> > IIUC, there can be a HOT-updated tuple which is not initially pruned
> > by heap_page_prune but later diagnosed HEAPTUPLE_DEAD by
> > HeapTupleSatisfiesVacuum (Since OldestXmin can be updated by the time
> > we call HeapTupleSatisfiesVacuum and xmax becomes older than
> > OldestXmin).
>
> Hm? OldestXmin is constant over the course of vacuum, no?
>
Yeah, it's constant. And, my understanding was completely wrong.

Actually, I misunderstood the following commit message:

commit dd69597988859c51131e0cbff3e30432db4259e1
Date: Thu May 2 10:07:13 2019 -0400

Fix some problems with VACUUM (INDEX_CLEANUP FALSE)

...
Change the logic for the case where a tuple is not initially pruned
by heap_page_prune but later diagnosed HEAPTUPLE_DEAD by
HeapTupleSatisfiesVacuum.
.....
I thought the reason is OldestXmin will be updated in between, but
that's just a stupid assumption I've made. :-(

I still need to understand the scenario that the above commit is
referring to. If those kind of tuples can't have committed xmax older
than OldestXmin/cutoff_xid, then we're good. Otherwise, these kind of
tuples can create problems if we're performing vacuum with index
cleanup disabled. Because, if index cleanup is disabled, we set
tupgone as false although the status of the tuple is HEAPTUPLE_DEAD
and try to freeze those tuple later.

You've also mentioned that HEAPTUPLE_DEAD case I'm referring to can
only be hit for for tuples that are *newer* than OldestXmin but become
DEAD (instead of RECENTLY_DEAD) because the inserting transaction
aborted. But, I don't see that's the only case when
HeapTupleSatisfiesVacuum returns HEAPTUPLE_DEAD. If
HeapTupleSatisfiesVacuumHorizon returns HEAPTUPLE_RECENTLY_DEAD and if
tuple xmax(dead_after) precedes OlestXmin, we set it as
HEAPTUPLE_DEAD.

res = HeapTupleSatisfiesVacuumHorizon(htup, buffer, &dead_after);

if (res == HEAPTUPLE_RECENTLY_DEAD)
{
Assert(TransactionIdIsValid(dead_after));

if (TransactionIdPrecedes(dead_after, OldestXmin))
res = HEAPTUPLE_DEAD;
}
else
Assert(!TransactionIdIsValid(dead_after));

Am I missing something here?

--
Thanks & Regards,
Kuntal Ghosh