Thread: Suspicious check (src/backend/access/gin/gindatapage.c)
At line 650 I can read:
--
cpp-today.blogspot.com
if ((leaf->lsize - segsize) - (leaf->lsize - segsize) < BLCKSZ / 4)
break;
I believe one of the two should be leaf->rsize
cpp-today.blogspot.com
On Fri, Sep 12, 2014 at 7:35 AM, Gaetano Mendola <mendola@gmail.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) This has been introduced by 36a35c5 and should be backpatched to 9.4. Regards, -- Michael
Attachment
On 09/12/2014 03:48 AM, Michael Paquier wrote: > On Fri, Sep 12, 2014 at 7:35 AM, Gaetano Mendola <mendola@gmail.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
Attachment
On 09/12/2014 11:38 AM, Heikki Linnakangas wrote: > 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. Actually, we should add a fillfactor reloption to GIN. But that's 9.5 material. - Heikki
On Fri, Sep 12, 2014 at 5:42 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote: > On 09/12/2014 11:38 AM, Heikki Linnakangas wrote: >> >> 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. > > > Actually, we should add a fillfactor reloption to GIN. But that's 9.5 > material. Actually gin is the only method that does not have this parameter, no? Then the following extra-processing would be enough I imagine: 1) Tune freespace using fillfactor when placing keys to leaf data page (dataPlaceToPageLeaf) 2) On split, instead of (BLCKSZ * 3) / 4, have the left leaf full at (BLCKSZ * fillfactor) / 100 Regards, -- Michael