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

On Wed, Dec 18, 2024 at 1:23 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
> Hi.
>
> Em qua., 18 de dez. de 2024 às 14:01, Melanie Plageman <melanieplageman(at)gmail(dot)com> escreveu:
>>
>> For now, I've committed the version of the patch I proposed above (v3).
>
> What happens if *rs_tuples* equal to zero in function *SampleHeapTupleVisible*?
> end = hscan->rs_ntuples - 1;

Ah yes, it seems like just changing the top if statement to
if (scan->rs_flags & SO_ALLOW_PAGEMODE &&
hscan->rs_ntuples > 0)

Thanks for identifying this.

> Would be good to fix the binary search too, now that unsigned types are used.

You just mean the one in SampleHeapTupleVisible(), right?

> Patch attached.

I'm not sure the attached patch is quite right because if rs_ntuples
is 0, it should still enter the first if statement and then return
false. In fact, with your patch, I think we would incorrectly not
return a value when rs_ntuples is 0 from SampleHeapTupleVisible().

How about one of these options:

option 1:
most straightforward fix

diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index d0e5922eed7..fa03bedd4b8 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2577,6 +2577,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,

if (scan->rs_flags & SO_ALLOW_PAGEMODE)
{
+ int start, end;
+
+ if (hscan->rs_ntuples == 0)
+ return false;
+
/*
* In pageatatime mode, heap_prepare_pagescan() already did visibility
* checks, so just look at the info it left in rs_vistuples[].
@@ -2586,8 +2591,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
* in increasing order, but it's not clear that there would be enough
* gain to justify the restriction.
*/
- int start = 0,
- end = hscan->rs_ntuples - 1;
+ start = 0;
+ end = hscan->rs_ntuples - 1;

while (start <= end)
{

option 2:
change the binary search code a bit more but avoid the extra branch
introduced by option 1.

diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index d0e5922eed7..c8e25bdd197 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2586,18 +2586,17 @@ SampleHeapTupleVisible(TableScanDesc scan,
Buffer buffer,
* in increasing order, but it's not clear that there would be enough
* gain to justify the restriction.
*/
- int start = 0,
- end = hscan->rs_ntuples - 1;
+ uint32 start = 0, end = hscan->rs_ntuples;

- while (start <= end)
+ while (start < end)
{
- int mid = (start + end) / 2;
+ int mid = (start + end) / 2;
OffsetNumber curoffset = hscan->rs_vistuples[mid];

if (tupoffset == curoffset)
return true;
else if (tupoffset < curoffset)
- end = mid - 1;
+ end = mid;
else
start = mid + 1;
}

- Melanie