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

Hi,

On 2022-03-03 16:22:11 -0800, Peter Geoghegan wrote:
> Attached is (roughly speaking) a rebased version of this patch (i.e.
> your v4-0004- patch from the patch series). Like your patch, this
> patch documents our expectations for vistest and OldestXmin. Unlike
> your patch, it makes all vacrel initialization take place before
> lazy_scan_heap is called, from inside heap_vacuum_rel.

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...


> An important detail here is how OldestXmin (and vistest) are related
> to rel_pages -- I also added something about that. It definitely
> wouldn't be okay to (say) move the call to RelationGetNumberOfBlocks
> to any place before the call to vacuum_set_xid_limits. And so it very
> much seems in scope here.

You're right, it needs to be that way round.


> I'm a bit confused about at least one detail here: you've said here
> that "GlobalVisTestFor() doesn't itself do any horizon determination",
> which seems to contradict comments from the v4-0004- patch that was
> attached to the same email.

Which comment in the patch is the quoted statement contradicting? The on-point
comment seems to be

+        * [...] vacrel->vistest is always at least as aggressive as the
+        * limits vacuum_set_xid_limits() computes because ComputeXidHorizons()
+        * (via vacuum_set_xid_limits() ->GetOldestNonRemovableTransactionId())
+        * ensures the approximate horizons are always at least as aggressive as
+        * the precise horizons.



> + * Also, we delay establishing vistest + * as a minor optimization.  A
> later cutoff can enable more eager pruning.  + */

I don't think the minor optimization does anything (which I had stated wrongly
at some point in this thread).

>  	/*
>  	 * Call lazy_scan_heap to perform all required heap pruning, index
> -	 * vacuuming, and heap vacuuming (plus related processing)
> +	 * vacuuming, and heap vacuuming (plus related processing).
> +	 *
> +	 * Note: Resource management for parallel VACUUM is quite subtle, and so
> +	 * lazy_scan_heap must allocate (and deallocate) vacrel->dead_items space
> +	 * for itself.  Every other vacrel field is initialized by now, though.
>  	 */
>  	lazy_scan_heap(vacrel, params->nworkers);

What are you referring to with "quite subtle"?


Greetings,

Andres Freund