Re: nbtree's ScalarArrayOp array mark/restore code appears to be buggy

On Sat, Sep 23, 2023 at 11:47 AM Peter Geoghegan <pg@bowt.ie> wrote:
> The fix for this should be fairly straightforward. We must teach
> _bt_restore_array_keys() to distinguish "past the end of the array"
> from "after the start of the array", so that doesn't spuriously skip a
> required call to _bt_preprocess_keys() . I already see that the
> problem goes away once _bt_restore_array_keys() is made to call
> _bt_preprocess_keys() unconditionally, so I'm already fairly confident
> that this will work.

Attached draft patch shows how this could work.

_bt_restore_array_keys() has comments that seem to suppose that
calling _bt_preprocess_keys is fairly expensive, and something that's
well worth avoiding. But...is it, really? I wonder if we'd be better
off just biting the bullet and always calling _bt_preprocess_keys
here. Could it really be such a big cost, compared to pinning and
locking the page in the btrestpos() path?

The current draft SAOP patch calls _bt_preprocess_keys() with a buffer
lock held. This is very likely unsafe, so I'll need to come up with a
principled approach (e.g. a stripped down, specialized version of
_bt_preprocess_keys that's safe to call while holding a lock seems doable).
I've been able to put that off for a few months now because it just
doesn't impact my microbenchmarks to any notable degree (and not just
in those cases where we can use the "numberOfKeys == 1" fast path at
the start of _bt_preprocess_keys). This at least suggests that the cost of
always calling _bt_preprocess_keys in _bt_restore_array_keys might be
acceptable.

--
Peter Geoghegan