On Tue, Mar 18, 2025 at 1:01 PM Matthias van de Meent <boekewurm+postg...@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
