Re: Can rs_cindex be < 0 for bitmap heap scans? - Mailing list pgsql-hackers

On Wed, Dec 18, 2024 at 1:23 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Hi.
>
> Em qua., 18 de dez. de 2024 às 14:01, Melanie Plageman <melanieplageman@gmail.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;
        }