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

Thanks for the reply, Dilip!

On Thu, Oct 24, 2024 at 12:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Oct 24, 2024 at 3:45 AM Melanie Plageman <melanieplageman@gmail.com> wrote:
>>
>> HeapScanDescData->rs_cindex (the current index into the array of
>> visible tuples stored in the heap scan descriptor) is used for
>> multiple scan types, but for bitmap heap scans, AFAICT, it should
>> never be < 0.
>
> You are right it can never be < 0 in this case at least.

Cool, thanks for confirming. I will commit this change then.

> In fact you don't need to explicitly set it to 0 in initscan[1], because before calling
heapam_scan_bitmap_next_tuple()we must call heapam_scan_bitmap_next_block() and this function is initializing this to 0
(hscan->rs_cindex= 0;).  Anyway no object even if you prefer to initialize in initscan(), just the point is that we are
alreadydoing it for each block before fetching tuples from the block. 

I am inclined to still initialize it to 0 in initscan(). I was
refactoring the bitmap heap scan code to use the read stream API and
after moving some things around, this field was no longer getting
initialized in heapam_scan_bitmap_next_block(). While that may not be
a good reason to change it in this patch, most of the other fields in
the heap scan descriptor (like rs_cbuf and rs_cblock) are
re-initialized in initscan(), so it seems okay to do that there even
though it isn't strictly necessary on master.

- Melanie

On Sat, Oct 26, 2024 at 12:17 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Oct 25, 2024 at 10:07 PM Melanie Plageman <melanieplageman@gmail.com> wrote:
>>
>> On Fri, Oct 25, 2024 at 10:29 AM Melanie Plageman
>> <melanieplageman@gmail.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
thisindex 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