Re: Suspicious check (src/backend/access/gin/gindatapage.c)

On 09/12/2014 03:48 AM, Michael Paquier wrote:
> On Fri, Sep 12, 2014 at 7:35 AM, Gaetano Mendola <mendola(at)gmail(dot)com> wrote:
>> At line 650 I can read:
>>
>> if ((leaf->lsize - segsize) - (leaf->lsize - segsize) < BLCKSZ / 4)
>> break;
>>
>> I believe one of the two should be leaf->rsize
> Yes this condition is broken. Shouldn't it be that instead when
> appending items at the end of a page?
> if ((leaf->lsize - segsize) - (leaf->rsize + segsize) < BLCKSZ / 4)

Yep, that's what it was supposed to be. The consequence was that when
appending to the index, the left page was always packed as tight as
possible. Which is not actually that bad a behavior, in fact if you're
just appending values and never do any other updates, it's optimal.
That's probably why no-one has noticed. But it's not what was intended.

But now that I look at it more closely, fixing this as you suggested
doesn't actually yield the intended behavior either. The intention was
to fill the left page 75% full. However, after fixing that typo, the
code would move items to the left page so that the difference between
the halves is BLCKSZ / 4, which does not give a 75% ratio. For example,
if there are 8200 bytes of data in total, the "fixed" code would
actually put 5124 bytes on the left page, and 3076 bytes on the right,
which makes the left page only 62% full.

Fixed, per the attached patch. Now that the logic is fixed, I hope we
won't get complaints that the indexes are bigger, if you fill a table by
appending to the end. I wonder if we should aim at an even more uneven
split; the default fillfactor for B-trees is 90%, for example. I didn't
go that high when I wrote that, because the code in previous versions
always did a 50/50 split. But it could be argued that a higher
fillfactor makes sense for a GIN index - they typically don't get as
much random updates as a B-tree.

- Heikki