Re: Fixing a couple of buglets in how VACUUM sets visibility map bits

Hi,

On 2023-01-09 10:16:03 -0800, Peter Geoghegan wrote:
> Attached is v3, which explicitly checks the need to set the PD_ALL_VISIBLE
> flag at the relevant visibilitymap_set() call site. It also has improved
> comments.

Afaict we'll need to backpatch this all the way?


> From e7788ebdb589fb7c6f866cf53658cc369f9858b5 Mon Sep 17 00:00:00 2001
> From: Peter Geoghegan <pg@bowt.ie>
> Date: Sat, 31 Dec 2022 15:13:01 -0800
> Subject: [PATCH v3] Don't accidentally unset all-visible bit in VM.
> 
> One of the calls to visibilitymap_set() during VACUUM's initial heap
> pass could unset a page's all-visible bit during the process of setting
> the same page's all-frozen bit.

As just mentioned upthread, this just seems wrong.


> This could happen in the event of a
> concurrent HOT update from a transaction that aborts soon after.  Since
> the all_visible_according_to_vm local variable that lazy_scan_heap works
> off of when setting the VM doesn't reflect the current state of the VM,
> and since visibilitymap_set() just requested that the all-frozen bit get
> set in one case, there was a race condition.  Heap pages could initially
> be all-visible just as all_visible_according_to_vm is established, then
> not be all-visible after the update, and then become eligible to be set
> all-visible once more following pruning by VACUUM.  There is no reason
> why VACUUM can't remove a concurrently aborted heap-only tuple right
> away, and so no reason why such a page won't be able to reach the
> relevant visibilitymap_set() call site.

Do you have a reproducer for this?


> @@ -1120,8 +1123,8 @@ lazy_scan_heap(LVRelState *vacrel)
>           * got cleared after lazy_scan_skip() was called, so we must recheck
>           * with buffer lock before concluding that the VM is corrupt.
>           */
> -        else if (all_visible_according_to_vm && !PageIsAllVisible(page)
> -                 && VM_ALL_VISIBLE(vacrel->rel, blkno, &vmbuffer))
> +        else if (all_visible_according_to_vm && !PageIsAllVisible(page) &&
> +                 visibilitymap_get_status(vacrel->rel, blkno, &vmbuffer) != 0)
>          {
>              elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation \"%s\" page
%u",
>                   vacrel->relname, blkno);

Hm. The message gets a bit less accurate with the change. Perhaps OK? OTOH, it
might be useful to know what bit was wrong when debugging problems.


> @@ -1164,12 +1167,34 @@ lazy_scan_heap(LVRelState *vacrel)
>                   !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer))
>          {
>              /*
> -             * We can pass InvalidTransactionId as the cutoff XID here,
> -             * because setting the all-frozen bit doesn't cause recovery
> -             * conflicts.
> +             * Avoid relying on all_visible_according_to_vm as a proxy for the
> +             * page-level PD_ALL_VISIBLE bit being set, since it might have
> +             * become stale -- even when all_visible is set in prunestate.
> +             *
> +             * Consider the example of a page that starts out all-visible and
> +             * then has a tuple concurrently deleted by an xact that aborts.
> +             * The page will be all_visible_according_to_vm, and will have
> +             * all_visible set in prunestate.  It will nevertheless not have
> +             * PD_ALL_VISIBLE set by here (plus neither VM bit will be set).
> +             * And so we must check if PD_ALL_VISIBLE needs to be set.
>               */
> +            if (!PageIsAllVisible(page))
> +            {
> +                PageSetAllVisible(page);
> +                MarkBufferDirty(buf);
> +            }
> +
> +            /*
> +             * Set the page all-frozen (and all-visible) in the VM.
> +             *
> +             * We can pass InvalidTransactionId as our visibility_cutoff_xid,
> +             * since a snapshotConflictHorizon sufficient to make everything
> +             * safe for REDO was logged when the page's tuples were frozen.
> +             */
> +            Assert(!TransactionIdIsValid(prunestate.visibility_cutoff_xid));
>              visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr,
>                                vmbuffer, InvalidTransactionId,
> +                              VISIBILITYMAP_ALL_VISIBLE    |
>                                VISIBILITYMAP_ALL_FROZEN);
>          }
>  
> @@ -1311,7 +1336,11 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block,
>  
>          /* DISABLE_PAGE_SKIPPING makes all skipping unsafe */
>          if (!vacrel->skipwithvm)
> +        {
> +            /* Caller shouldn't rely on all_visible_according_to_vm */
> +            *next_unskippable_allvis = false;
>              break;
> +        }
>  
>          /*
>           * Aggressive VACUUM caller can't skip pages just because they are
> @@ -1818,7 +1847,11 @@ retry:
>               * cutoff by stepping back from OldestXmin.
>               */
>              if (prunestate->all_visible && prunestate->all_frozen)
> +            {
> +                /* Using same cutoff when setting VM is now unnecessary */
>                  snapshotConflictHorizon = prunestate->visibility_cutoff_xid;
> +                prunestate->visibility_cutoff_xid = InvalidTransactionId;
> +            }
>              else
>              {
>                  /* Avoids false conflicts when hot_standby_feedback in use */
> @@ -2388,8 +2421,8 @@ lazy_vacuum_all_indexes(LVRelState *vacrel)
>  static void
>  lazy_vacuum_heap_rel(LVRelState *vacrel)
>  {
> -    int            index;
> -    BlockNumber vacuumed_pages;
> +    int            index = 0;
> +    BlockNumber vacuumed_pages = 0;
>      Buffer        vmbuffer = InvalidBuffer;
>      LVSavedErrInfo saved_err_info;
>  
> @@ -2406,42 +2439,42 @@ lazy_vacuum_heap_rel(LVRelState *vacrel)
>                               VACUUM_ERRCB_PHASE_VACUUM_HEAP,
>                               InvalidBlockNumber, InvalidOffsetNumber);
>  
> -    vacuumed_pages = 0;
> -
> -    index = 0;
>      while (index < vacrel->dead_items->num_items)
>      {
> -        BlockNumber tblk;
> +        BlockNumber blkno;
>          Buffer        buf;
>          Page        page;
>          Size        freespace;
>  
>          vacuum_delay_point();

I still think such changes are inappropriate for a bugfix, particularly one
that needs to be backpatched.

Greetings,

Andres Freund