Re: BUG #17255: Server crashes in index_delete_sort_cmp() due to race condition with vacuum

Hi,

On 2022-03-10 18:03:46 -0800, Peter Geoghegan wrote:
> Attached is v2 revision of the less critical vacuumlazy.c vistest
> patch. I will respond to your second email about the pruneheap.c patch
> on a new dedicated thread, per your suggestion.
> 
> NB: I intend to commit this revision of the patch (or something pretty
> close to it) in the next few days, barring any objections.

WFM.


> On Wed, Mar 9, 2022 at 4:25 PM Andres Freund <andres@anarazel.de> wrote:
> > I think it'd be nicer if we did the horizon determination after allocating
> > space for dead tuples... But it's still better than today, so...
> 
> Why would it be nicer?

Large allocation can take a bit. Especially dead_item_alloc() sneakily
initializes parallelism (which is darn ugly). Determining the horizon after
doing expensive stuff gives you a slightly better horizon...



>  	/*
> +	 * Now actually update rel's pg_class entry.
> +	 *
>  	 * Aggressive VACUUM must reliably advance relfrozenxid (and relminmxid).
>  	 * We are able to advance relfrozenxid in a non-aggressive VACUUM too,
>  	 * provided we didn't skip any all-visible (not all-frozen) pages using
> @@ -787,7 +832,7 @@ static void
>  lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  {
>  	VacDeadItems *dead_items;
> -	BlockNumber nblocks,
> +	BlockNumber rel_pages = vacrel->rel_pages,
>  				blkno,
>  				next_unskippable_block,
>  				next_failsafe_block,

> @@ -843,7 +865,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  
>  	/* Report that we're scanning the heap, advertising total # of blocks */
>  	initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP;
> -	initprog_val[1] = nblocks;
> +	initprog_val[1] = rel_pages;
>  	initprog_val[2] = dead_items->max_items;
>  	pgstat_progress_update_multi_param(3, initprog_index, initprog_val);
>  
> @@ -867,9 +889,9 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  	 * Before entering the main loop, establish the invariant that
>  	 * next_unskippable_block is the next block number >= blkno that we can't
>  	 * skip based on the visibility map, either all-visible for a regular scan
> -	 * or all-frozen for an aggressive scan.  We set it to nblocks if there's
> -	 * no such block.  We also set up the skipping_blocks flag correctly at
> -	 * this stage.
> +	 * or all-frozen for an aggressive scan.  We set it to rel_pages when
> +	 * there's no such block.  We also set up the skipping_blocks flag
> +	 * correctly at this stage.
>  	 *
>  	 * Note: The value returned by visibilitymap_get_status could be slightly
>  	 * out-of-date, since we make this test before reading the corresponding
> @@ -887,7 +909,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  	next_unskippable_block = 0;
>  	if (vacrel->skipwithvm)
>  	{
> -		while (next_unskippable_block < nblocks)
> +		while (next_unskippable_block < rel_pages)
>  		{
>  			uint8		vmstatus;
>  
> @@ -914,7 +936,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  	else
>  		skipping_blocks = false;
>  
> -	for (blkno = 0; blkno < nblocks; blkno++)
> +	for (blkno = 0; blkno < rel_pages; blkno++)
>  	{
>  		Buffer		buf;
>  		Page		page;
> @@ -932,7 +954,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  			next_unskippable_block++;
>  			if (vacrel->skipwithvm)
>  			{
> -				while (next_unskippable_block < nblocks)
> +				while (next_unskippable_block < rel_pages)
>  				{
>  					uint8		vmskipflags;
>  
> @@ -977,7 +999,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  			/*
>  			 * The current page can be skipped if we've seen a long enough run
>  			 * of skippable blocks to justify skipping it -- provided it's not
> -			 * the last page in the relation (according to rel_pages/nblocks).
> +			 * the last page in the relation (according to rel_pages).
>  			 *
>  			 * We always scan the table's last page to determine whether it
>  			 * has tuples or not, even if it would otherwise be skipped. This
> @@ -985,7 +1007,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  			 * on the table to attempt a truncation that just fails
>  			 * immediately because there are tuples on the last page.
>  			 */
> -			if (skipping_blocks && blkno < nblocks - 1)
> +			if (skipping_blocks && blkno < rel_pages - 1)
>  			{
>  				/*
>  				 * Tricky, tricky.  If this is in aggressive vacuum, the page
> @@ -1153,7 +1175,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  		 * were pruned some time earlier.  Also considers freezing XIDs in the
>  		 * tuple headers of remaining items with storage.
>  		 */
> -		lazy_scan_prune(vacrel, buf, blkno, page, vistest, &prunestate);
> +		lazy_scan_prune(vacrel, buf, blkno, page, &prunestate);
>  
>  		Assert(!prunestate.all_visible || !prunestate.has_lpdead_items);
>  
> @@ -1352,7 +1374,7 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers)
>  	vacrel->blkno = InvalidBlockNumber;
>  
>  	/* now we can compute the new value for pg_class.reltuples */
> -	vacrel->new_live_tuples = vac_estimate_reltuples(vacrel->rel, nblocks,
> +	vacrel->new_live_tuples = vac_estimate_reltuples(vacrel->rel, rel_pages,
>  													 vacrel->scanned_pages,
>  													 vacrel->live_tuples);

The whole s/nblocks/rel_pages/ seems like it should be done separately.

Greetings,

Andres Freund