Re: Combine Prune and Freeze records emitted by vacuum

On 01/04/2024 19:08, Melanie Plageman wrote:
> On Mon, Apr 01, 2024 at 05:17:51PM +0300, Heikki Linnakangas wrote:
>> diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
>> @@ -256,15 +270,16 @@ heap_page_prune(Relation relation, Buffer buffer,
>> tup.t_tableOid = RelationGetRelid(relation);
>>
>> /*
>> - * Determine HTSV for all tuples.
>> + * Determine HTSV for all tuples, and queue them up for processing as HOT
>> + * chain roots or as a heap-only items.
>
> Reading this comment now as a whole, I would add something like
> "Determining HTSV for all tuples once is required for correctness" to
> the start of the second paragraph. The new conjunction on the first
> paragraph sentence followed by the next paragraph is a bit confusing
> because it sounds like queuing them up for processing is required for
> correctness (which, I suppose it is on some level). Basically, I'm just
> saying that it is now less clear what is required for correctness.

Fixed.

> While I recognize this is a matter of style and not important, I
> personally prefer this for reverse looping:
>
> for (int i = prstate.nheaponly_items; i --> 0;)

I don't think we use that style anywhere in the Postgres source tree
currently. (And I don't like it ;-) )

> I do think a comment about the reverse order would be nice. I know it
> says something above the first loop to this effect:
>
> * Processing the items in reverse order (and thus the tuples in
> * increasing order) increases prefetching efficiency significantly /
> * decreases the number of cache misses.
>
> So perhaps we could just say "as above, process the items in reverse
> order"

I'm actually not sure why it makes a difference. I would assume all the
data to already be in CPU cache at this point, since the first loop
already accessed it, so I think there's something else going on. But I
didn't investigate it deeper. Anyway, added a comment.

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?

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.

With the next patches, heap_prune_record_unchanged() will do more, and
will also throw an error on a HEAPTUPLE_LIVE tuple, so even though in
the first patch we could print just a WARNING and move on, it gets more
awkward with the rest of the patches.

(Continuing with the remaining patches..)

--
Heikki Linnakangas
Neon (https://neon.tech)