Re: Fillfactor for GIN indexes

In dataPlaceToPageLeaf-function:

>     if (append)
>     {
>         /*
>          * Even when appending, trying to append more items than will fit is
>          * not completely free, because we will merge the new items and old
>          * items into an array below. In the best case, every new item fits in
>          * a single byte, and we can use all the free space on the old page as
>          * well as the new page. For simplicity, ignore segment overhead etc.
>          */
>         maxitems = Min(maxitems, freespace + GinDataPageMaxDataSize);
>     }

Hmm. So after splitting the page, there is freespace + 
GinDataPageMaxDataSize bytes available on both pages together. freespace 
has been adjusted with the fillfactor, but GinDataPageMaxDataSize is 
not. So this overshoots, because when leafRepackItems actually 
distributes the segments on the pages, it will fill both pages only up 
to the fillfactor. This is an upper bound, so that's harmless, it only 
leads to some unnecessary work in dealing with the item lists. But I 
think that should be:

maxitems = Min(maxitems, freespace + leaf->maxdatasize);

>     else
>     {
>         /*
>          * Calculate a conservative estimate of how many new items we can fit
>          * on the two pages after splitting.
>          *
>          * We can use any remaining free space on the old page to store full
>          * segments, as well as the new page. Each full-sized segment can hold
>          * at least MinTuplesPerSegment items
>          */
>         int            nnewsegments;
>
>         nnewsegments = freespace / GinPostingListSegmentMaxSize;
>         nnewsegments += GinDataPageMaxDataSize / GinPostingListSegmentMaxSize;
>         maxitems = Min(maxitems, nnewsegments * MinTuplesPerSegment);
>     }

This branch makes the same mistake, but this is calculating a lower 
bound. It's important that maxitems is not set to higher value than what 
actually fits on the page, otherwise you can get an ERROR later in the 
function, when it turns out that not all the items actually fit on the 
page. The saving grace here is that this branch is never taken when 
building a new index, because index build inserts all the TIDs in order, 
but that seems pretty fragile. Should use leaf->maxdatasize instead of 
GinDataPageMaxDataSize here too.

But that can lead to funny things, if fillfactor is small, and BLCKSZ is 
small too. With the minimums, BLCKSZ=1024 and fillfactor=0.2, the above 
formula will set nnewsegments to zero. That's not going to end up well. 
The problem is that maxdatasize becomes smaller than 
GinPostingListSegmentMaxSize, which is a problem. I think 
GinGetMaxDataSize() needs to make sure that the returned value is always >= GinPostingListSegmentMaxSize.

Now that we have a fillfactor, shouldn't we replace the 75% heuristic 
later in that function, when inserting to the rightmost page rather than 
building it from scratch? In B-tree, the fillfactor is applied to that 
case too.

- Heikki