Re: Combine Prune and Freeze records emitted by vacuum - Mailing list pgsql-hackers

On Mon, Apr 01, 2024 at 05:17:51PM +0300, Heikki Linnakangas wrote:
> On 30/03/2024 07:57, Melanie Plageman wrote:
> 
> > The final state of the code could definitely use more cleanup. I've been
> > staring at it for awhile, so I could use some thoughts/ideas about what
> > part to focus on improving.
> 
> Committed some of the changes. I plan to commit at least the first of these
> remaining patches later today. I'm happy with it now, but I'll give it a
> final glance over after dinner.
> 
> I'll continue to review the rest after that, but attached is what I have
> now.

Review for 0003-0006 (I didn't have any new thoughts on 0002). I know
you didn't modify them much/at all, but I noticed some things in my code
that could be better.

> From 17e183835a968e81daf7b74a4164b243e2de35aa Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Fri, 29 Mar 2024 19:43:09 -0400
> Subject: [PATCH v11 3/7] Introduce PRUNE_DO_* actions
> 
> We will eventually take additional actions in heap_page_prune() at the
> discretion of the caller. For now, introduce these PRUNE_DO_* macros and
> turn mark_unused_now, a paramter to heap_page_prune(), into a PRUNE_DO_

paramter -> parameter

> action.
> ---
>  src/backend/access/heap/pruneheap.c  | 51 ++++++++++++++--------------
>  src/backend/access/heap/vacuumlazy.c | 11 ++++--
>  src/include/access/heapam.h          | 13 ++++++-
>  3 files changed, 46 insertions(+), 29 deletions(-)
> 
> diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
> index fb0ad834f1b..30965c3c5a1 100644
> --- a/src/backend/access/heap/pruneheap.c
> +++ b/src/backend/access/heap/pruneheap.c
> @@ -29,10 +29,11 @@
>  /* Working data for heap_page_prune and subroutines */
>  typedef struct
>  {
> +    /* PRUNE_DO_* arguments */
> +    uint8        actions;

I wasn't sure if actions is a good name. What do you think?

> +
>      /* tuple visibility test, initialized for the relation */
>      GlobalVisState *vistest;
> -    /* whether or not dead items can be set LP_UNUSED during pruning */
> -    bool        mark_unused_now;
>  
>      TransactionId new_prune_xid;    /* new prune hint value for page */
>      TransactionId snapshotConflictHorizon;    /* latest xid removed */

> diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
> index 32a3fbce961..35b8486c34a 100644
> --- a/src/include/access/heapam.h
> +++ b/src/include/access/heapam.h
> @@ -191,6 +191,17 @@ typedef struct HeapPageFreeze
>  
>  } HeapPageFreeze;
>  
> +/*
> + * Actions that can be taken during pruning and freezing. By default, we will
> + * at least attempt regular pruning.
> + */
> +
> +/*
> + * PRUNE_DO_MARK_UNUSED_NOW indicates whether or not dead items can be set
> + * LP_UNUSED during pruning.
> + */
> +#define        PRUNE_DO_MARK_UNUSED_NOW (1 << 1)


No reason for me to waste the zeroth bit here. I just realized that I
did this with XLHP_IS_CATALOG_REL too.

#define        XLHP_IS_CATALOG_REL            (1 << 1)

> From 083690b946e19ab5e536a9f2689772e7b91d2a70 Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Fri, 29 Mar 2024 21:22:14 -0400
> Subject: [PATCH v11 4/7] Prepare freeze tuples in heap_page_prune()
> 
> diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
> index 35b8486c34a..ac129692c13 100644
> --- a/src/include/access/heapam.h
> +++ b/src/include/access/heapam.h
>  
> +/*
> + * Prepare to freeze if advantageous or required and try to advance
> + * relfrozenxid and relminmxid. To attempt freezing, we will need to determine
> + * if the page is all frozen. So, if this action is set, we will also inform
> + * the caller if the page is all-visible and/or all-frozen and calculate a

I guess we don't inform the caller if the page is all-visible, so this
is not quite right.

> + * snapshot conflict horizon for updating the visibility map.
> + */
> +#define        PRUNE_DO_TRY_FREEZE (1 << 2)

> From ef8cb2c089ad9474a6da309593029c08f71b0bb9 Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Fri, 29 Mar 2024 21:36:37 -0400
> Subject: [PATCH v11 5/7] Set hastup in heap_page_prune
> 
> diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
> index 8bdd6389b25..65b0ed185ff 100644
> --- a/src/backend/access/heap/pruneheap.c
> +++ b/src/backend/access/heap/pruneheap.c
> @@ -66,6 +66,9 @@ typedef struct
>      bool        processed[MaxHeapTuplesPerPage + 1];
>  
>      int            ndeleted;        /* Number of tuples deleted from the page */
> +
> +    /* Whether or not the page makes rel truncation unsafe */
> +    bool        hastup;
>  } PruneState;
>  
>  /* Local functions */
> @@ -271,6 +274,7 @@ heap_page_prune(Relation relation, Buffer buffer,
>      prstate.ndeleted = 0;
>      prstate.nroot_items = 0;
>      prstate.nheaponly_items = 0;
> +    prstate.hastup = false;
>  
>      /*
>       * If we will prepare to freeze tuples, consider that it might be possible
> @@ -280,7 +284,7 @@ heap_page_prune(Relation relation, Buffer buffer,
>          presult->all_frozen = true;
>      else
>          presult->all_frozen = false;
> -
> +    presult->hastup = prstate.hastup;
>  
>      /*
>       * presult->htsv is not initialized here because all ntuple spots in the
> @@ -819,6 +823,8 @@ heap_prune_record_redirect(PruneState *prstate,
>       */
>      if (was_normal)
>          prstate->ndeleted++;
> +
> +    prstate->hastup = true;
>  }
>  
>  /* Record line pointer to be marked dead */
> @@ -901,11 +907,15 @@ static void
>  heap_prune_record_unchanged_lp_normal(Page page, int8 *htsv, PruneState *prstate,
>                                        PruneResult *presult, OffsetNumber offnum)
>  {
> -    HeapTupleHeader htup = (HeapTupleHeader) PageGetItem(page, PageGetItemId(page, offnum));
> +    HeapTupleHeader htup;
>  
>      Assert(!prstate->processed[offnum]);
>      prstate->processed[offnum] = true;
>  
> +    presult->hastup = true;        /* the page is not empty */

My fault, but hastup being set sometimes in PruneState and sometimes in
PruneResult is quite unpalatable.


> From dffa90d0bd8e972cfe26da96860051dc8c2a8576 Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Sat, 30 Mar 2024 01:27:08 -0400
> Subject: [PATCH v11 6/7] Save dead tuple offsets during heap_page_prune
> ---
> diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
> index 212d76045ef..7f1e4db55c0 100644
> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> @@ -1373,6 +1373,15 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
>      return false;
>  }
>  
> +static int
> +OffsetNumber_cmp(const void *a, const void *b)
> +{
> +    OffsetNumber na = *(const OffsetNumber *) a,
> +                nb = *(const OffsetNumber *) b;
> +
> +    return na < nb ? -1 : na > nb;
> +}

This probably doesn't belong here. I noticed spgdoinsert.c had a static
function for sorting OffsetNumbers, but I didn't see anything general
purpose anywhere else.

- Melanie