On Thu, Feb 13, 2025 at 11:28 AM Tomas Vondra <to...@vondra.me> wrote: > > On 2/13/25 17:01, Melanie Plageman wrote: > > On Thu, Feb 13, 2025 at 10:46 AM Tomas Vondra <to...@vondra.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
