Thread: Heap's backwards scan scans the incorrect pages with heap_setscanlimits()

Hackers,

It looks like both heapgettup() and heapgettup_pagemode() are coded
incorrectly when setting the page to start the scan on for a backwards
scan when heap_setscanlimits() has been used.

It looks like the code was not updated during 7516f5259.

The current code is:

/* start from last page of the scan */
if (scan->rs_startblock > 0)
    page = scan->rs_startblock - 1;
else
    page = scan->rs_nblocks - 1;


Where rs_startblock is either the sync scan start location, or the
start page set by heap_setscanlimits().  rs_nblocks is the number of
blocks in the relation.

Let's say we have a 100 block relation and we want to scan blocks 10
to 30 in a backwards order. We'll do heap_setscanlimits(scan, 10, 21);
to indicate that we want to scan 21 blocks starting at page 10 and
finishing after scanning page 30.

What the code above does is wrong.  Since rs_startblock is > 0 we'll execute:

page = scan->rs_nblocks - 1;

i.e. 99. then proceed to scan blocks all blocks down to 78.  Oops. Not
quite the 10 to 30 that we asked for.

Now, it does not appear that there are any live bugs here, in core at
least.  The only usage I see of heap_setscanlimits() is in
heapam_index_build_range_scan() to which I see the scan is a forward
scan. I only noticed the bug as I'm in the middle of fixing up [1] to
implement backwards TID Range scans.

Proposed patch attached.

Since this is not a live bug, is it worth a backpatch?  I guess some
extensions could suffer from this, I'm just not sure how likely that
is as if anyone is doing backwards scanning with a setscanlimits set,
then they'd surely have noticed this already!?

David

[1] https://postgr.es/m/CAMyN-kB-nFTkF=VA_JPwFNo08S0d-Yk0F741S2B7LDmYAi8eyA@mail.gmail.com

On Thu, 21 Jan 2021 at 13:16, David Rowley <dgrowleyml@gmail.com> wrote:
> Proposed patch attached.

I ended up pushing a slightly revised version of this which kept the
code the same as before when rs_numblocks had not been changed. I
backpatched to 9.5 as it seemed low risk and worthy of stopping some
head-scratching and a future report for any extension authors that
make use of heap_setscanlimits() with backwards scans at some point in
the future.

David