Re: Incorrect assumption in heap_prepare_freeze_tuple

On Sat, Oct 3, 2020 at 12:05 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
Thank you for your quick response and detailed explanation.

>
> > * It is assumed that the caller has checked the tuple with
> > * HeapTupleSatisfiesVacuum() and determined that it is not HEAPTUPLE_DEAD
> > * (else we should be removing the tuple, not freezing it).
> >
> > Thus, when we see a committed xmax that precedes the cutoff_xid, we throw
> > the following data corruption error:
> > errmsg_internal("cannot freeze committed xmax %u", xid)
> >
> > However, in the caller (lazy_scan_heap), HeapTupleSatisfiesVacuum may
> > return HEAPTUPLE_DEAD for an updated/deleted tuple that got modified by a
> > transaction older than OldestXmin. And, if the tuple is HOT-updated, it
> > should only be removed by a hot-chain prune operation. So, we treat the
> > tuple as RECENTLY_DEAD and don't remove the tuple.
>
> This code is so terrible :(
>
> We really should just merge the HOT pruning and lazy_scan_heap()
> removal/freeze operations. That'd avoid this corner case and *also*
> would significantly reduce the WAL volume of VACUUM. And safe a good bit
> of CPU time.
>
+1

>
> > So, it may lead to an incorrect data corruption error. IIUC, following will
> > be the exact scenario where the error may happen,
> >
> > An updated/deleted tuple whose xamx is in between cutoff_xid and
> > OldestXmin. Since cutoff_xid depends on vacuum_freeze_min_age and
> > autovacuum_freeze_max_age, it'll not be encountered easily. But, I think
> > it can be reproduced with some xid burner patch.
>
> I don't think this case is possible (*). By definition, there cannot be any
> transactions needing tuples from before OldestXmin. Which means that the
> heap_page_prune() earlier in lazy_scan_heap() would have pruned away a
> DEAD tuple version that is part of a hot chain.
>
> The HEAPTUPLE_DEAD branch you're referring to really can only be hit for
> tuples that are *newer* than OldestXmin but become DEAD (instead of
> RECENTLY_DEAD) because the inserting transaction aborted.
>
>
> (*) with the exception of temp tables due to some recent changes, I am
> currently working on a fix for that.
>
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). So, there can be deleted tuples with xmax older than
OldestXmin. But, you're right that any tuple older than cutoff_xid
(since it was set earlier) will be pruned by heap_page_prune and hence
we shouldn't encounter the error. I'll study the rewrite_heap_tuple
path as well.

> What made you look at this? Did you hit the error?
Nope, I haven't encountered the error. Just trying to understand the code. :-)

--
Thanks & Regards,
Kuntal Ghosh