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

Em qui., 19 de dez. de 2024 às 19:50, Melanie Plageman <
melanieplageman(at)gmail(dot)com> escreveu:

> On Wed, Dec 18, 2024 at 9:50 PM Richard Guo <guofenglinux(at)gmail(dot)com>
> wrote:
> >
> > On Thu, Dec 19, 2024 at 8:18 AM Melanie Plageman
> > <melanieplageman(at)gmail(dot)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.
>
Look goods to me.
I think that you propose, can get rid of the early test too.
Note the way we can avoid an overflow in the mid calculation.

diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index 9f17baea5d..bd1335276a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2577,9 +2577,6 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer
buffer,
uint32 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[].
@@ -2590,17 +2587,17 @@ SampleHeapTupleVisible(TableScanDesc scan, Buffer
buffer,
* gain to justify the restriction.
*/
start = 0;
- end = hscan->rs_ntuples - 1;
+ end = hscan->rs_ntuples;

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

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

best regards,
Ranier Vilela