On Tue, Mar 18, 2025 at 1:01 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> 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.
We don't leak any memory here. We temporarily switch over to using
so->arrayContext, for the duration of these calls. And so the memory
will be freed on rescan -- there's a MemoryContextReset call at the
top of _bt_preprocess_array_keys to take care of this, which is hit on
rescan.
> 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.
I'll see if I can do something like that for the next revision, but I
think that it might be more awkward than it's worth.
The duplication isn't quite duplication, so much as it's two functions
that are mirror images of each other. The details/direction of things
is flipped in a number of places. The strategy number differs, even
though the same function is called.
--
Peter Geoghegan
