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