Re: Combine Prune and Freeze records emitted by vacuum

On Mon, Apr 1, 2024 at 1:37 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> Committed the first of the remaining patches with those changes. And
> also this, which is worth calling out:
>
> > if (prstate.htsv[offnum] == HEAPTUPLE_DEAD)
> > {
> > ItemId itemid = PageGetItemId(page, offnum);
> > HeapTupleHeader htup = (HeapTupleHeader) PageGetItem(page, itemid);
> >
> > if (likely(!HeapTupleHeaderIsHotUpdated(htup)))
> > {
> > HeapTupleHeaderAdvanceConflictHorizon(htup,
> > &prstate.latest_xid_removed);
> > heap_prune_record_unused(&prstate, offnum, true);
> > }
> > else
> > {
> > /*
> > * This tuple should've been processed and removed as part of
> > * a HOT chain, so something's wrong. To preserve evidence,
> > * we don't dare to remove it. We cannot leave behind a DEAD
> > * tuple either, because that will cause VACUUM to error out.
> > * Throwing an error with a distinct error message seems like
> > * the least bad option.
> > */
> > elog(ERROR, "dead heap-only tuple (%u, %d) is not linked to from any HOT chain",
> > blockno, offnum);
> > }
> > }
> > else
> > heap_prune_record_unchanged_lp_normal(page, &prstate, offnum);
>
> As you can see, I turned that into a hard error. Previously, that code
> was at the top of heap_prune_chain(), and it was normal to see DEAD
> heap-only tuples there, because they would presumably get processed
> later as part of a HOT chain. But now this is done after all the HOT
> chains have already been processed.
>
> Previously if there was a dead heap-only tuple like that on the page for
> some reason, it was silently not processed by heap_prune_chain()
> (because it was assumed that it would be processed later as part of a
> HOT chain), and was left behind as a HEAPTUPLE_DEAD tuple. If the
> pruning was done as part of VACUUM, VACUUM would fail with "ERROR:
> unexpected HeapTupleSatisfiesVacuum result". Or am I missing something?

I think you are right. I wasn't sure if there was some way for a HOT,
DEAD tuple to be not HOT-updated, but that doesn't make much sense.

> Now you get that above error also on on-access pruning, which is not
> ideal. But I don't remember hearing about corruption like that, and
> you'd get the error on VACUUM anyway.

Yea, that makes sense. One thing I don't really understand is why
vacuum has its own system for saving and restoring error information
for context messages (LVSavedErrInfo and
update/restore_vacuum_err_info()). I'll confess I don't know much
about how error cleanup works in any sub-system. But it stuck out to
me that vacuum has its own. I assume it is okay to add new error
messages and they somehow will work with the existing system?

- Melanie