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