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

On Sat, Oct 26, 2024 at 5:30 PM Melanie Plageman <melanieplageman@gmail.com>
wrote:

> 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 this index as +ve (Forward
> scan )as well as -ve (Backward scan).
>
> 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.


> 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).


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com