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

On Mon, 17 Mar 2025 at 23:51, Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Tue, 11 Mar 2025 at 16:53, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> >
> > On Sat, Mar 8, 2025 at 11:43 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> > > I plan on committing this one soon. It's obviously pretty pointless to
> > > make the BTMaxItemSize operate off of a page header, and not requiring
> > > it is more flexible.
> >
> > Committed. And committed a revised version of "Show index search count
> > in EXPLAIN ANALYZE" that addresses the issues with non-parallel-aware
> > index scan executor nodes that run from a parallel worker.
> >
> > Attached is v28. This is just to keep the patch series applying
> > cleanly -- no real changes here.
>
> You asked off-list for my review of 0003. I'd already reviewed 0001
> before that, so that review also included. I'll see if I can spend
> some time on the other patches too

My comments on 0004:

> _bt_skiparray_strat_decrement
> _bt_skiparray_strat_increment

In both functions the generated value isn't used when the in/decrement
overflows (and thus invalidates the qual), or when the opclass somehow
doesn't have a <= or >= operator, respectively.
For byval types that's not much of an issue, but for by-ref types
(such as uuid, or bigint on 32-bit systems) that's not great, as btree
explicitly allows no leaks for the in/decrement functions, and now we
use those functions and leak the values.

Additionally, the code is essentially duplicated between the
functions, with as only differences which sksup function to call;
which opstrategies to check, and where to retrieve/put the value. It's
only 2 instances total, but if you figure out how to make a nice
single function from the two that'd be appreciated, as it reduces
duplication and chances for divergence.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)