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

Hi,

On 2023-01-08 14:39:19 -0800, Peter Geoghegan wrote:
> 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.

How? visibilitymap_set() just adds flags, it doesn't remove any already
existing bits:

        map[mapByte] |= (flags << mapOffset);

It'll afaict lead to potentially unnecessary WAL records though, which does
seem buggy:
    if (flags != (map[mapByte] >> mapOffset & VISIBILITYMAP_VALID_BITS))

here we check for *equivalence*, but then below we just or-in flags. So
visibilitymap_set() with just one of the flags bits set in the parameters,
but both set in the page would end up WAL logging unnecessarily.




> @@ -2388,8 +2398,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 +2416,42 @@ lazy_vacuum_heap_rel(LVRelState *vacrel)
>                               VACUUM_ERRCB_PHASE_VACUUM_HEAP,
>                               InvalidBlockNumber, InvalidOffsetNumber);
>  
> -    vacuumed_pages = 0;
> -
> -    index = 0;


> @@ -2473,12 +2484,12 @@ lazy_vacuum_heap_rel(LVRelState *vacrel)
>   */
>  static int
>  lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer,
> -                      int index, Buffer *vmbuffer)
> +                      int index, Buffer vmbuffer)
>  {
>      VacDeadItems *dead_items = vacrel->dead_items;
>      Page        page = BufferGetPage(buffer);
>      OffsetNumber unused[MaxHeapTuplesPerPage];
> -    int            uncnt = 0;
> +    int            nunused = 0;
>      TransactionId visibility_cutoff_xid;
>      bool        all_frozen;
>      LVSavedErrInfo saved_err_info;
> @@ -2508,10 +2519,10 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer,
>  
>          Assert(ItemIdIsDead(itemid) && !ItemIdHasStorage(itemid));
>          ItemIdSetUnused(itemid);
> -        unused[uncnt++] = toff;
> +        unused[nunused++] = toff;
>      }
>  
> -    Assert(uncnt > 0);
> +    Assert(nunused > 0);
>  
>      /* Attempt to truncate line pointer array now */
>      PageTruncateLinePointerArray(page);
> @@ -2527,13 +2538,13 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer,
>          xl_heap_vacuum xlrec;
>          XLogRecPtr    recptr;
>  
> -        xlrec.nunused = uncnt;
> +        xlrec.nunused = nunused;
>  
>          XLogBeginInsert();
>          XLogRegisterData((char *) &xlrec, SizeOfHeapVacuum);
>  
>          XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
> -        XLogRegisterBufData(0, (char *) unused, uncnt * sizeof(OffsetNumber));
> +        XLogRegisterBufData(0, (char *) unused, nunused * sizeof(OffsetNumber));
>  
>          recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_VACUUM);
>  

You have plenty of changes like this, which are afaict entirely unrelated to
the issue the commit is fixing, in here. It just makes it hard to review the
patch.

Greetings,

Andres Freund