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

Hi.

Sorry for not responding quickly.
I have been without communication until now.

Em qua., 18 de dez. de 2024 às 17:13, Melanie Plageman <
melanieplageman(at)gmail(dot)com> escreveu:

> 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().
>
I'm wondering if *rs_tuples* is zero, would be run the second search
*HeapTupleSatisfiesVisibility*.

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

I'm looking at the commit and the replies in the thread.

best regards,
Ranier Vilela