Re: Adding skip scan (including MDAM style range skip scan) to nbtree

On Thu, Sep 12, 2024 at 10:49 AM Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:
> Thanks to Peter for the description, that helped me debug the issue. I
> think I found a fix for the issue: regression tests for 811af978
> consistently got stuck on my macbook before the attached patch 0001,
> after applying that this patch they completed just fine.

Thanks for taking a look at it.

> The fix in 0001 is relatively simple: we stop backends from waiting
> for a concurrent backend to resolve the NEED_PRIMSCAN condition, and
> instead move our local state machine so that we'll hit _bt_first
> ourselves, so that we may be able to start the next primitive scan.

I agree with your approach, but I'm concerned about it causing
confusion inside _bt_parallel_done. And so I attach a v2 revision of
your bug fix. v2 adds a check that nails that down, too. I'm not 100%
sure if the change to _bt_parallel_done becomes strictly necessary, to
make the basic fix robust, but it's a good idea either way. In fact, it
seemed like a good idea even before this bug came to light: it was
already clear that this was strictly necessary for the skip scan
patch. And for reasons that really have nothing to do with the
requirements for skip scan (it's related to how we call
_bt_parallel_done without much care in code paths from the original
parallel index scan commit).

More details on changes in v2 that didn't appear in Matthias' v1:

v2 makes _bt_parallel_done do nothing at all when the backend-local
so->needPrimScan flag is set (regardless of whether it has been set by
_bt_parallel_seize or by _bt_advance_array_keys). This is a bit like
the approach taken before the Postgres 17 work went in:
_bt_parallel_done used to only permit the shared btps_pageStatus state
to become BTPARALLEL_DONE when it found that "so->arrayKeyCount >=
btscan->btps_arrayKeyCount" (else the call was a no-op). With this
extra hardening, _bt_parallel_done will only permit setting BTPARALLEL_DONE when
"!so->needPrimScan". Same idea, more or less.

v2 also changes comments in _bt_parallel_seize. The comment tweaks
suggest that the new "if (!first && status ==
BTPARALLEL_NEED_PRIMSCAN) return false" path is similar to the
existing master branch "if (!first && so->needPrimScan) return false"
precheck logic on master (the precheck that takes place before
examining any state in shared memory). The new path can be thought of
as dealing with cases where the backend-local so->needPrimScan flag
must have been stale back when it was prechecked -- it's essentially the same
logic, though unlike the precheck it works against the authoritative
shared memory state.

My current plan is to commit something like this in the next day or two.

--
Peter Geoghegan