Re: Combine Prune and Freeze records emitted by vacuum

On Mon, Apr 1, 2024 at 1:37 PM Heikki Linnakangas <hlinnaka@iki.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