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. I don't want to bikeshed this - I'll leave the decision about 0001 to you. I'm OK with both options. regards -- Tomas Vondra
