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