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

On Sat, Oct 26, 2024 at 12:17 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Fri, Oct 25, 2024 at 10:07 PM Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
>>
>> On Fri, Oct 25, 2024 at 10:29 AM Melanie Plageman
>> <melanieplageman(at)gmail(dot)com> wrote:
>> >
>> > Tom suggested off-list that if rs_cindex can't be zero, then it should
>> > be unsigned. I checked the other scan types using the
>> > HeapScanDescData, and it seems none of them use values of rs_cindex or
>> > rs_ntuples < 0. As such, here is a patch making both rs_ntuples and
>> > rs_cindex unsigned.
>>
>
> @@ -943,8 +945,8 @@ heapgettup_pagemode(HeapScanDesc scan,
> {
> HeapTuple tuple = &(scan->rs_ctup);
> Page page;
> - int lineindex;
> - int linesleft;
> + uint32 lineindex;
> + uint32 linesleft;
>
> IMHO we can't make "lineindex" as uint32, because just see the first code block[1] of heapgettup_pagemode(), we use this index as +ve (Forward scan )as well as -ve (Backward scan).
>
> [1]
> if (likely(scan->rs_inited))
> {
> /* continue from previously returned page/tuple */
> page = BufferGetPage(scan->rs_cbuf);
>
> lineindex = scan->rs_cindex + dir;
> if (ScanDirectionIsForward(dir))
>
> --Refer definition of ScanDirection
> typedef enum ScanDirection
> {
> BackwardScanDirection = -1,
> NoMovementScanDirection = 0,
> ForwardScanDirection = 1
> } ScanDirection;

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.

We could add an if statement above the goto that says something like
if (linesleft > 0)
goto continue_page;

Would that make it clearer?

- Melanie