Re: Index Skip Scan

On Sat, Nov 2, 2019 at 11:56 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Wed, Sep 25, 2019 at 2:33 AM Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> > v27-0001-Index-skip-scan.patch
>
> Some random thoughts on this:

And now some more:

* I'm confused about this code in _bt_skip():

> /*
> * Andvance backward but read forward. At this moment we are at the next
> * distinct key at the beginning of the series. In case if scan just
> * started, we can read forward without doing anything else. Otherwise find
> * previous distinct key and the beginning of it's series and read forward
> * from there. To do so, go back one step, perform binary search to find
> * the first item in the series and let _bt_readpage do everything else.
> */
> else if (ScanDirectionIsBackward(dir) && ScanDirectionIsForward(indexdir))
> {
> if (!scanstart)
> {
> _bt_drop_lock_and_maybe_pin(scan, &so->currPos);
> offnum = _bt_binsrch(scan->indexRelation, so->skipScanKey, buf);
>
> /* One step back to find a previous value */
> _bt_readpage(scan, dir, offnum);

Why is it okay to call _bt_drop_lock_and_maybe_pin() like this? It
looks like that will drop the lock (but not the pin) on the same
buffer that you binary search with _bt_binsrch() (since the local
variable "buf" is also the buf in "so->currPos").

* It also seems a bit odd that you assume that the scan is
"scan->xs_want_itup", but then check that condition many times. Why
bother?

* Similarly, why bother using _bt_drop_lock_and_maybe_pin() at all,
rather than just unlocking the buffer directly? We'll only drop the
pin for a scan that is "!scan->xs_want_itup", which is never the case
within _bt_skip().

I think that the macros and stuff that manage pins and buffer locks in
nbtsearch.c is kind of a disaster anyway [1]. Maybe there is some
value in trying to be consistent with existing nbtsearch.c code in
ways that aren't strictly necessary.

* Not sure why you need this code after throwing an error:

> else
> {
> elog(ERROR, "Could not read closest index tuples: %d", offnum);
> pfree(so->skipScanKey);
> so->skipScanKey = NULL;
> return false;
> }

[1] https://www.postgresql.org/message-id/flat/CAH2-Wz=m674-RKQdCG+jCD9QGzN1Kcg-FOdYw4-j+5_PfcHbpQ(at)mail(dot)gmail(dot)com
--
Peter Geoghegan