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

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

From
Dilip Kumar
Date:
On Thu, Oct 24, 2024 at 3:45 AM Melanie Plageman <melanieplageman@gmail.com> wrote:
Hi,

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.

As such, I find this test in heapam_scan_bitmap_next_tuple() pretty confusing.

 if (hscan->rs_cindex < 0 || hscan->rs_cindex >= hscan->rs_ntuples)

I know it seems innocuous, but I find it pretty distracting. Am I
missing something? Could it somehow be < 0?

If not, I propose removing that part of the if statement like in the
attached patch.


You are right it can never be < 0 in this case at least.  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 already doing it for each block before fetching tuples from the block.

[1]
@@ -378,6 +378,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
  ItemPointerSetInvalid(&scan->rs_ctup.t_self);
  scan->rs_cbuf = InvalidBuffer;
  scan->rs_cblock = InvalidBlockNumber;
+ scan->rs_cindex = 0;

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

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

From
Melanie Plageman
Date:
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



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

From
Dilip Kumar
Date:
On Thu, Oct 24, 2024 at 7:11 PM Melanie Plageman <melanieplageman@gmail.com> wrote:
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.

+1
 

> 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 already doing 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.

 make sense

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

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

From
Dilip Kumar
Date:
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).

[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;

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

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

From
Melanie Plageman
Date:
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



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

From
Dilip Kumar
Date:
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