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

On Wed, Dec 18, 2024 at 9:50 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Thu, Dec 19, 2024 at 8:18 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > I pushed the straightforward option for now so that it's fixed.
>
> I think this binary search code now has a risk of underflow.  If 'mid'
> is calculated as zero, the second 'if' branch will cause 'end' to
> underflow.

Thanks, Richard!

> Maybe we need to do something like below.
>
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -2600,7 +2600,11 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
>             if (tupoffset == curoffset)
>                 return true;
>             else if (tupoffset < curoffset)
> +           {
> +               if (mid == 0)
> +                   return false;
>                 end = mid - 1;
> +           }
>             else
>                 start = mid + 1;
>         }
>
> Alternatively, we can revert 'start' and 'end' to signed int as they
> were before.

What about this instead:

diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index 2da4e4da13e..fb90fd869c2 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2574,11 +2574,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,

    if (scan->rs_flags & SO_ALLOW_PAGEMODE)
    {
-       uint32      start,
-                   end;
-
-       if (hscan->rs_ntuples == 0)
-           return false;
+       uint32      start = 0,
+                   end = hscan->rs_ntuples;

        /*
         * In pageatatime mode, heap_prepare_pagescan() already did visibility
@@ -2589,10 +2586,8 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
         * in increasing order, but it's not clear that there would be enough
         * gain to justify the restriction.
         */
-       start = 0;
-       end = hscan->rs_ntuples - 1;

-       while (start <= end)
+       while (start < end)
        {
            uint32      mid = (start + end) / 2;
            OffsetNumber curoffset = hscan->rs_vistuples[mid];
@@ -2600,7 +2595,7 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer buffer,
            if (tupoffset == curoffset)
                return true;
            else if (tupoffset < curoffset)
-               end = mid - 1;
+               end = mid;
            else
                start = mid + 1;
        }

Or to make it easier to read, here:

        uint32        start = 0,
              end = hscan->rs_ntuples;

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

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

I think this gets rid of any subtraction and is still the same.