Re: Combine Prune and Freeze records emitted by vacuum

On 25/01/2024 00:49, Melanie Plageman wrote:
> Generates 30% fewer WAL records and 12% fewer WAL bytes -- which,
> depending on what else is happening on the system, can lead to vacuum
> spending substantially less time on WAL writing and syncing (often 15%
> less time on WAL writes and 10% less time on syncing WAL in my
> testing).

Nice!

> The attached patch set is broken up into many separate commits for
> ease of review. Each patch does a single thing which can be explained
> plainly in the commit message. Every commit passes tests and works on
> its own.

About this very first change:

> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> @@ -1526,8 +1526,7 @@ lazy_scan_prune(LVRelState *vacrel,
>  					 * that everyone sees it as committed?
>  					 */
>  					xmin = HeapTupleHeaderGetXmin(htup);
> -					if (!TransactionIdPrecedes(xmin,
> -											   vacrel->cutoffs.OldestXmin))
> +					if (!GlobalVisTestIsRemovableXid(vacrel->vistest, xmin))
>  					{
>  						all_visible = false;
>  						break;

Does GlobalVisTestIsRemovableXid() handle FrozenTransactionId correctly?

I read through all the patches in order, and aside from the above they 
all look correct to me. Some comments on the set as whole:

I don't think we need XLOG_HEAP2_FREEZE_PAGE as a separate record type 
anymore. By removing that, you also get rid of the freeze-only codepath 
near the end of heap_page_prune(), and the 
heap_freeze_execute_prepared() function.

The XLOG_HEAP2_FREEZE_PAGE record is a little smaller than 
XLOG_HEAP2_PRUNE. But we could optimize the XLOG_HEAP2_PRUNE format for 
the case that there's no pruning, just freezing. The record format 
(xl_heap_prune) looks pretty complex as it is, I think it could be made 
both more compact and more clear with some refactoring.

FreezeMultiXactId still takes a separate 'cutoffs' arg, but it could use 
pagefrz->cutoffs now.

HeapPageFreeze has two "trackers", for the "freeze" and "no freeze" 
cases. heap_page_prune() needs to track both, until it decides whether 
to freeze or not. But it doesn't make much sense that the caller 
(lazy_scan_prune()) has to initialize both, and has to choose which of 
the values to use after the call depending on whether heap_page_prune() 
froze or not. The two trackers should be just heap_page_prune()'s 
internal business.

HeapPageFreeze is a bit confusing in general, as it's both an input and 
an output to heap_page_prune(). Not sure what exactly to do there, but I 
feel that we should make heap_page_prune()'s interface more clear. 
Perhaps move the output fields to PruneResult.

Let's rename heap_page_prune() to heap_page_prune_and_freeze(), as 
freezing is now an integral part of the function. And mention it in the 
function comment, too.

In heap_prune_chain:

>  * Tuple visibility information is provided in presult->htsv.

Not this patch's fault directly, but it's not immediate clear what "is 
provided" means here. Does the caller provide it, or does the function 
set it, ie. is it an input or output argument? Looking at the code, it's 
an input, but now it looks a bit weird that an input argument is called 
'presult'.

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