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