Re: Combine Prune and Freeze records emitted by vacuum

On 15/03/2024 02:56, Melanie Plageman wrote:
> Okay, so I was going to start using xl_heap_prune for vacuum here too,
> but I realized it would be bigger because of the
> snapshotConflictHorizon. Do you think there is a non-terrible way to
> make the snapshotConflictHorizon optional? Like with a flag?

Yeah, another flag would do the trick.

>> I introduced a few sub-record types similar to what you suggested --
>> they help a bit with alignment, so I think they are worth keeping. There
>> are comments around them, but perhaps a larger diagram of the layout of
>> a the new XLOG_HEAP2_PRUNE record would be helpful.
> 
> I started doing this, but I can't find a way of laying out the diagram
> that pgindent doesn't destroy. I thought I remember other diagrams in
> the source code showing the layout of something (something with pages
> somewhere?) that don't get messed up by pgindent, but I couldn't find
> them.

See src/tools/pgindent/README, section "Cleaning up in case of failure 
or ugly output":

     /*----------
      * Text here will not be touched by pgindent.
      */

>> There is a bit of duplicated code between heap_xlog_prune() and
>> heap2_desc() since they both need to deserialize the record. Before the
>> code to do this was small and it didn't matter, but it might be worth
>> refactoring it that way now.
> 
> I have added a helper function to do the deserialization,
> heap_xlog_deserialize_prune_and_freeze(). But I didn't start using it in
> heap2_desc() because of the way the pg_waldump build file works. Do you
> think the helper belongs in any of waldump's existing sources?
> 
>     pg_waldump_sources = files(
>         'compat.c',
>         'pg_waldump.c',
>         'rmgrdesc.c',
>     )
> 
>     pg_waldump_sources += rmgr_desc_sources
>     pg_waldump_sources += xlogreader_sources
>     pg_waldump_sources += files('../../backend/access/transam/xlogstats.c')
> 
> Otherwise, I assume I am suppose to avoid adding some big new include to
> waldump.

Didn't look closely at that, but there's some precedent with 
commit/prepare/abort records. See ParseCommitRecord, xl_xact_commit, 
xl_parsed_commit et al.

Note that we don't provide WAL compatibility across major versions. You 
can fully remove the old xl_heap_freeze_page format. (We should bump 
XLOG_PAGE_MAGIC when this is committed though)

>> On the point of removing the freeze-only code path from
>> heap_page_prune() (now heap_page_prune_and_freeze()): while doing this,
>> I realized that heap_pre_freeze_checks() was not being called in the
>> case that we decide to freeze because we emitted an FPI while setting
>> the hint bit. I've fixed that, however, I've done so by moving
>> heap_pre_freeze_checks() into the critical section. I think that is not
>> okay? I could move it earlier and not do call it when the hint bit FPI
>> leads us to freeze tuples. But, I think that would lead to us doing a
>> lot less validation of tuples being frozen when checksums are enabled.
>> Or, I could make two critical sections?
> 
> I found another approach and just do the pre-freeze checks if we are
> considering freezing except for the FPI criteria.

Hmm, I think you can make this simpler if you use 
XLogCheckBufferNeedsBackup() to check if the hint-bit update will 
generate a full-page-image before entering the critical section. Like 
you did to check if pruning will generate a full-page-image. I included 
that change in the attached patches.

I don't fully understand this:

>     /*
>      * If we will freeze tuples on the page or they were all already frozen
>      * on the page, if we will set the page all-frozen in the visibility map,
>      * we can advance relfrozenxid and relminmxid to the values in
>      * pagefrz->FreezePageRelfrozenXid and pagefrz->FreezePageRelminMxid.
>      */
>     if (presult->all_frozen || presult->nfrozen > 0)
>     {
>         presult->new_relfrozenxid = pagefrz->FreezePageRelfrozenXid;
>         presult->new_relminmxid = pagefrz->FreezePageRelminMxid;
>     }
>     else
>     {
>         presult->new_relfrozenxid = pagefrz->NoFreezePageRelfrozenXid;
>         presult->new_relminmxid = pagefrz->NoFreezePageRelminMxid;
>     }

Firstly, the comment is outdated, because we have already done the 
freezing at this point. But more importantly, I don't understand the 
logic here. Could we check just presult->nfrozen > 0 here and get the 
same result?

>> I think it is probably worse to add both of them as additional optional
>> arguments, so I've just left lazy_scan_prune() with the job of
>> initializing them.

Ok.


Here are some patches on top of your patches for some further 
refactorings. Some notable changes in heap_page_prune_and_freeze():

- I moved the heap_prepare_freeze_tuple() work from the 2nd loop to the 
1st one. It seems more clear and efficient that way.

- I extracted the code to generate the WAL record to a separate function.

- Refactored the "will setting hint bit generate FPI" check as discussed 
above

These patches are in a very rough WIP state, but I wanted to share 
early. I haven't done much testing, and I'm not wedded to these changes, 
but I think they make it more readable. Please include / squash in the 
patch set if you agree with them.

Please also take a look at the comments I marked with HEIKKI or FIXME, 
in the patches and commit messages.

I'll wait for a new version from you before reviewing more.

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