Re: Bug in nbtree optimization to skip > operator comparisons (or < comparisons in backwards scans)

Hi, Peter!

On Sat, Dec 9, 2023 at 4:29 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> Does this really need to work at the scan key level, rather than at
> the whole-scan level? Wouldn't it make more sense to just totally
> disable it for the whole scan, since we'll barely ever need to do that
> anyway?
>
> My ScalarArrayOpExpr patch will need to disable this optimization,
> since with that patch in place we don't necessarily go through
> _bt_first each time the search-type scan keys must change. We might
> need to check a few tuples from before the _bt_first-wise position of
> the next set of array values, which is a problem with
> opposite-direction-only inequalities (it's a little bit like the
> situation from my test case, actually). That's partly why I'd prefer
> this to work at the whole-scan level (though I also just don't think
> that inventing SK_BT_BT_FIRST makes much sense).
>
> I think that you should make it clearer that this whole optimization
> only applies to required *inequalities*, which can be required in the
> opposite direction *only*. It should be more obvious from looking at
> the code that the optimization doesn't apply to required equality
> strategy scan keys (those are always required in *both* scan
> directions or in neither direction, so unlike the closely related
> prefix optimization added by the same commit, they just can't use the
> optimization that we need to fix here).
>
> BTW, do we really need to keep around the BTScanOpaqueData.firstPage
> field? Why can't the call to _bt_readpage from _bt_first (and from
> _bt_endpoint) just pass "firstPage=true" as a simple argument? Note
> that the first call to _bt_readpage must take place from _bt_first (or
> from _bt_endpoint). The first _bt_first call is already kind of
> special, in a way that is directly related to this issue. I added some
> comments about that to today's commit c9c0589fda, in fact -- I think
> it's an important issue in general.

Please, check the attached patchset.

The first patch removes the BTScanOpaqueData.firstPage field as you
proposed. I think this is a good idea, thank you for the proposal.

Regarding the requiredOppositeDir bug. I don't want to lose the
generality of the optimization. I could work for different cases, for
example.
WHERE col1 > val1 AND col1 < val2
WHERE col1 = val1 AND col2 > val2 AND col2 < val3
WHERE col1 = val1 AND col2 = val2 AND col3 > val3 AND col3 < val4
And there could be additional scan keys, which shouldn't be skipped.
But that shouldn't mean we shouldn't skip others.

See the second patch for my next proposal to fix the problem. Instead
of relying on _bt_first(), let's rely on the first matched item on the
page. Once it's found, we may skip scan keys required for the opposite
direction. What do you think?

------
Regards,
Alexander Korotkov