Re: Eliminate redundant tuple visibility check in vacuum - Mailing list pgsql-hackers

Hi,

On 2023-09-07 18:23:22 -0400, Melanie Plageman wrote:
> From e986940e546171d1f1d06f62a101d695a8481e7a Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Wed, 6 Sep 2023 14:57:20 -0400
> Subject: [PATCH v4 2/3] Move heap_page_prune output parameters into struct
> 
> Add PruneResult, a structure containing the output parameters and result
> of heap_page_prune(). Reorganizing the results of heap_page_prune() into
> a struct simplifies the function signature and provides a location for
> future commits to store additional output parameters.
> 
> Discussion: https://postgr.es/m/CAAKRu_br124qsGJieuYA0nGjywEukhK1dKBfRdby_4yY3E9SXA%40mail.gmail.com
> ---
>  src/backend/access/heap/pruneheap.c  | 33 +++++++++++++---------------
>  src/backend/access/heap/vacuumlazy.c | 17 +++++---------
>  src/include/access/heapam.h          | 13 +++++++++--
>  src/tools/pgindent/typedefs.list     |  1 +
>  4 files changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
> index 392b54f093..5ac286e152 100644
> --- a/src/backend/access/heap/pruneheap.c
> +++ b/src/backend/access/heap/pruneheap.c
> @@ -155,15 +155,13 @@ heap_page_prune_opt(Relation relation, Buffer buffer)
>           */
>          if (PageIsFull(page) || PageGetHeapFreeSpace(page) < minfree)
>          {
> -            int            ndeleted,
> -                        nnewlpdead;
> +            PruneResult presult;
>  
> -            ndeleted = heap_page_prune(relation, buffer, vistest,
> -                                       &nnewlpdead, NULL);
> +            heap_page_prune(relation, buffer, vistest, &presult, NULL);
>  
>              /*
>               * Report the number of tuples reclaimed to pgstats.  This is
> -             * ndeleted minus the number of newly-LP_DEAD-set items.
> +             * presult.ndeleted minus the number of newly-LP_DEAD-set items.
>               *
>               * We derive the number of dead tuples like this to avoid totally
>               * forgetting about items that were set to LP_DEAD, since they
> @@ -175,9 +173,9 @@ heap_page_prune_opt(Relation relation, Buffer buffer)
>               * tracks ndeleted, since it will set the same LP_DEAD items to
>               * LP_UNUSED separately.
>               */
> -            if (ndeleted > nnewlpdead)
> +            if (presult.ndeleted > presult.nnewlpdead)
>                  pgstat_update_heap_dead_tuples(relation,
> -                                               ndeleted - nnewlpdead);
> +                                               presult.ndeleted - presult.nnewlpdead);
>          }
>  
>          /* And release buffer lock */
> @@ -204,24 +202,22 @@ heap_page_prune_opt(Relation relation, Buffer buffer)
>   * (see heap_prune_satisfies_vacuum and
>   * HeapTupleSatisfiesVacuum).
>   *
> - * Sets *nnewlpdead for caller, indicating the number of items that were
> - * newly set LP_DEAD during prune operation.
> + * presult contains output parameters needed by callers such as the number of
> + * tuples removed and the number of line pointers newly marked LP_DEAD.
> + * heap_page_prune() is responsible for initializing it.
>   *
>   * off_loc is the current offset into the line pointer array while pruning.
>   * This is used by vacuum to populate the error context message. On-access
>   * pruning has no such callback mechanism for populating the error context, so
>   * it passes NULL. When provided by the caller, off_loc is set exclusively by
>   * heap_page_prune().
> - *
> - * Returns the number of tuples deleted from the page during this call.
>   */
> -int
> +void
>  heap_page_prune(Relation relation, Buffer buffer,
>                  GlobalVisState *vistest,
> -                int *nnewlpdead,
> +                PruneResult *presult,
>                  OffsetNumber *off_loc)
>  {
> -    int            ndeleted = 0;
>      Page        page = BufferGetPage(buffer);
>      BlockNumber blockno = BufferGetBlockNumber(buffer);
>      OffsetNumber offnum,
> @@ -247,6 +243,9 @@ heap_page_prune(Relation relation, Buffer buffer,
>      prstate.nredirected = prstate.ndead = prstate.nunused = 0;
>      memset(prstate.marked, 0, sizeof(prstate.marked));
>  
> +    presult->ndeleted = 0;
> +    presult->nnewlpdead = 0;
> +
>      maxoff = PageGetMaxOffsetNumber(page);
>      tup.t_tableOid = RelationGetRelid(prstate.rel);
>  
> @@ -321,7 +320,7 @@ heap_page_prune(Relation relation, Buffer buffer,
>              continue;
>  
>          /* Process this item or chain of items */
> -        ndeleted += heap_prune_chain(buffer, offnum, &prstate);
> +        presult->ndeleted += heap_prune_chain(buffer, offnum, &prstate);
>      }
>  
>      /* Clear the offset information once we have processed the given page. */
> @@ -422,9 +421,7 @@ heap_page_prune(Relation relation, Buffer buffer,
>      END_CRIT_SECTION();
>  
>      /* Record number of newly-set-LP_DEAD items for caller */
> -    *nnewlpdead = prstate.ndead;
> -
> -    return ndeleted;
> +    presult->nnewlpdead = prstate.ndead;
>  }
>  
>  
> diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
> index 102cc97358..7ead9cfe9d 100644
> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> @@ -1544,12 +1544,11 @@ lazy_scan_prune(LVRelState *vacrel,
>      ItemId        itemid;
>      HeapTupleData tuple;
>      HTSV_Result res;
> -    int            tuples_deleted,
> -                tuples_frozen,
> +    PruneResult presult;
> +    int            tuples_frozen,
>                  lpdead_items,
>                  live_tuples,
>                  recently_dead_tuples;
> -    int            nnewlpdead;
>      HeapPageFreeze pagefrz;
>      int64        fpi_before = pgWalUsage.wal_fpi;
>      OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
> @@ -1572,7 +1571,6 @@ retry:
>      pagefrz.FreezePageRelminMxid = vacrel->NewRelminMxid;
>      pagefrz.NoFreezePageRelfrozenXid = vacrel->NewRelfrozenXid;
>      pagefrz.NoFreezePageRelminMxid = vacrel->NewRelminMxid;
> -    tuples_deleted = 0;
>      tuples_frozen = 0;
>      lpdead_items = 0;
>      live_tuples = 0;
> @@ -1581,9 +1579,8 @@ retry:
>      /*
>       * Prune all HOT-update chains in this page.
>       *
> -     * We count tuples removed by the pruning step as tuples_deleted.  Its
> -     * final value can be thought of as the number of tuples that have been
> -     * deleted from the table.  It should not be confused with lpdead_items;
> +     * We count the number of tuples removed from the page by the pruning step
> +     * in presult.ndeleted. It should not be confused with lpdead_items;
>       * lpdead_items's final value can be thought of as the number of tuples
>       * that were deleted from indexes.
>       *
> @@ -1591,9 +1588,7 @@ retry:
>       * current offset when populating the error context message, so it is
>       * imperative that we pass its location to heap_page_prune.
>       */
> -    tuples_deleted = heap_page_prune(rel, buf, vacrel->vistest,
> -                                     &nnewlpdead,
> -                                     &vacrel->offnum);
> +    heap_page_prune(rel, buf, vacrel->vistest, &presult, &vacrel->offnum);
>  
>      /*
>       * Now scan the page to collect LP_DEAD items and check for tuples
> @@ -1933,7 +1928,7 @@ retry:
>      }
>  
>      /* Finally, add page-local counts to whole-VACUUM counts */
> -    vacrel->tuples_deleted += tuples_deleted;
> +    vacrel->tuples_deleted += presult.ndeleted;
>      vacrel->tuples_frozen += tuples_frozen;
>      vacrel->lpdead_items += lpdead_items;
>      vacrel->live_tuples += live_tuples;
> diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
> index 6598c4d7d8..2d3f149e4f 100644
> --- a/src/include/access/heapam.h
> +++ b/src/include/access/heapam.h
> @@ -191,6 +191,15 @@ typedef struct HeapPageFreeze
>  
>  } HeapPageFreeze;
>  
> +/*
> + * Per-page state returned from pruning
> + */
> +typedef struct PruneResult
> +{
> +    int            ndeleted;        /* Number of tuples deleted from the page */
> +    int            nnewlpdead;        /* Number of newly LP_DEAD items */
> +} PruneResult;

I think it might be worth making the names a bit less ambiguous than they
were. It's a bit odd that one has "new" in the name, the other doesn't,
despite both being about newly marked things. And "deleted" seems somewhat
ambiguous, it could also be understood as marking things LP_DEAD. Maybe
nnewunused?


>  static int    heap_prune_chain(Buffer buffer,
>                               OffsetNumber rootoffnum,
> +                             int8 *htsv,
>                               PruneState *prstate);

Hm, do we really want to pass this explicitly to a bunch of functions? Seems
like it might be better to either pass the PruneResult around or to have a
pointer in PruneState?


>          /*
>           * The criteria for counting a tuple as live in this block need to
> @@ -1682,7 +1664,7 @@ retry:
>           * (Cases where we bypass index vacuuming will violate this optimistic
>           * assumption, but the overall impact of that should be negligible.)
>           */
> -        switch (res)
> +        switch ((HTSV_Result) presult.htsv[offnum])
>          {
>              case HEAPTUPLE_LIVE:

I think we should assert that we have a valid HTSV_Result here, i.e. not
-1. You could wrap the cast and Assert into an inline funciton as well.

Greetings,

Andres Freund