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)