Re: BitmapHeapScan streaming read user and prelim refactoring

On Thu, Feb 13, 2025 at 11:28 AM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> On 2/13/25 17:01, Melanie Plageman wrote:
> > On Thu, Feb 13, 2025 at 10:46 AM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> >>
> >> I reviewed v29 today, and I think it's pretty much ready to go.
> >>
> >> The one part where I don't quite get is 0001, which replaces a
> >> FLEXIBLE_ARRAY_MEMBER array with a fixed-length array. It's not wrong,
> >> but I don't quite see the benefits / clarity. And I think Thomas might
> >> be right we may want to make this dynamic, to save memory.
> >>
> >> Not a blocker, but I'd probably skip 0001 (unless it's required by the
> >> later parts, I haven't checked/tried).
> >
> > So, on master, it already pallocs an array of size MAX_TUPLES_PER_PAGE
> > (which is hard-coded in the tidbitmap API to MaxHeapTuplesPerPage) --
> > see tbm_begin_private_iterate().
> >
> > So we always palloc the same amount of memory. The reason I changed it
> > from a flexible sized array to a fixed size is that we weren't using
> > the flexibility and having a flexible sized array in the
> > TBMIterateResult meant it couldn't be nested in another struct. Since
> > I have to separate the TBMIterateResult and TBMIterator to implement
> > the read stream API for BHS, once I separate them, I nest the
> > TBMIterateResult in the GinScanEntry. If the array of offsets is
> > flexible sized, then I would have to manage that memory separately in
> > GIN code for the TBMIterateResult..
> >
> > So, 0001 isn't a change in the amount of memory allocated.
> >
> > With the read stream API, these TBMIterateResults are palloc'd just
> > like we palloc'd one in master. However, we have to have more than one
> > at a time.
> >
>
> I know it's not changing how much memory we allocate (compared to
> master). I haven't thought about the GinScanEntry - yes, flexible array
> member would make this a bit more complex.

Oh, I see. I didn't understand Thomas' proposal. I don't know how hard
it would be to make tidbitmap allocate the offsets on-demand. I'd need
to investigate more. But probably not worth it for this patch.

- Melanie