Re: Index Skip Scan - Mailing list pgsql-hackers

On Sat, Nov 2, 2019 at 11:56 AM Peter Geoghegan <pg@bowt.ie> wrote:
> On Wed, Sep 25, 2019 at 2:33 AM Dmitry Dolgov <9erthalion6@gmail.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@mail.gmail.com
--
Peter Geoghegan