Re: Combine Prune and Freeze records emitted by vacuum

On 20/03/2024 23:03, Melanie Plageman wrote:
> On Wed, Mar 20, 2024 at 03:15:49PM +0200, Heikki Linnakangas wrote:
>> diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
>> index ee0eca8ae02..b2015f5a1ac 100644
>> --- a/src/include/access/heapam.h
>> +++ b/src/include/access/heapam.h
>> @@ -202,14 +202,17 @@ typedef struct PruneFreezeResult
>> int recently_dead_tuples;
>> int ndeleted; /* Number of tuples deleted from the page */
>> int nnewlpdead; /* Number of newly LP_DEAD items */
>> + int nfrozen;
>
> Let's add a comment after int nfrozen like
> /* Number of newly frozen tuples */
>
>> +
>> bool all_visible; /* Whether or not the page is all visible */
>> bool hastup; /* Does page make rel truncation unsafe */
>>
>> + /* The following fields are only set if freezing */
>
> So, all_frozen will be set correctly if we are even considering freezing
> (if pagefrz is passed). all_frozen will be true even if we didn't freeze
> anything if the page is all-frozen and can be set as such in the VM. And
> it will be false if the page is not all-frozen. So, maybe we say
> "considering freezing".
>
> But, I'm glad you thought to call out which of these fields will make
> sense to the caller.
>
> Also, maybe we should just name the members to which this applies. It is
> a bit confusing that I can't tell if the comment also refers to the
> other members following it (lpdead_items and deadoffsets) -- which it
> doesn't.

Right, sorry, I spotted the general issue that it's not clear which
fields are valid when. I added that comment to remind about that, but I
then forgot about it.

In heap_page_prune_and_freeze(), we now do some extra work on each live
tuple, to set the all_visible_except_removable correctly. And also to
update live_tuples, recently_dead_tuples and hastup. When we're not
freezing, that's a waste of cycles, the caller doesn't care. I hope it's
enough that it doesn't matter, but is it?

The first commit (after the WAL format changes) changes the all-visible
check to use GlobalVisTestIsRemovableXid. The commit message says that
it's because we don't have 'cutoffs' available, but we only care about
that when we're freezing, and when we're freezing, we actually do have
'cutoffs' in HeapPageFreeze. Using GlobalVisTestIsRemovableXid seems
sensible anyway, because that's what we use in
heap_prune_satisfies_vacuum() too, but just wanted to point that out.

The 'frz_conflict_horizon' stuff is still fuzzy to me. (Not necessarily
these patches's fault). This at least is wrong, because Max(a, b)
doesn't handle XID wraparound correctly:

> if (do_freeze)
> conflict_xid = Max(prstate.snapshotConflictHorizon,
> presult->frz_conflict_horizon);
> else
> conflict_xid = prstate.snapshotConflictHorizon;

Then there's this in lazy_scan_prune():

> /* Using same cutoff when setting VM is now unnecessary */
> if (presult.all_frozen)
> presult.frz_conflict_horizon = InvalidTransactionId;
This does the right thing in the end, but if all the tuples are frozen
shouldn't frz_conflict_horizon already be InvalidTransactionId? The
comment says it's "newest xmin on the page", and if everything was
frozen, all xmins are FrozenTransactionId. In other words, that should
be moved to heap_page_prune_and_freeze() so that it doesn't lie to its
caller. Also, frz_conflict_horizon is only set correctly if
'all_frozen==true', would be good to mention that in the comments too.

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