Re: Can rs_cindex be < 0 for bitmap heap scans?

On Mon, Oct 28, 2024 at 2:27 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Sat, Oct 26, 2024 at 5:30 PM Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
>>
>> Yes, so in the case of backwards scan, if scan->rs_cindex is 0, when
>> dir is -1, lineindex will wrap around, but we don't use it in that
>> case because linesleft will be 0 and then we will not meet the
>> conditions to execute the code in the loop under continue_page. And in
>> the loop, when adding -1 to lineindex, for backwards scan, it always
>> starts at linesleft and ticks down to 0.
>
>
> Yeah you are right, although the lineindex will wrap around when rs_cindex is 0 , it would not be used. So, it won’t actually cause any issues, but I’m not comfortable with the unintentional wraparound. I would have left "scan->rs_cindex" as int itself but I am fine with whatever you prefer.

So, I don't see -1 as different from the wrapped around value with an
unsigned data type. Also neither is a valid number of entries in
rs_vistuples (which is limited to MaxHeapTuplesPerPage).

That being said, I can see how having lineindex be an invalid value
could be confusing -- either with an unsigned or signed data type for
rs_cindex. I think to make this clear we would have to add another
special case for backwards and no movement scan.

For now, I've committed the version of the patch I proposed above (v3).

>> We could add an if statement above the goto that says something like
>> if (linesleft > 0)
>> goto continue_page;
>>
>> Would that make it clearer?
>
>
> Not sure it would make it clearer. In fact, In common cases it would add an extra instruction to check the if (linesleft > 0).

Yes, indeed.

Thank you for taking a look and being so responsive on this thread.

- Melanie