On 09/12/2014 03:48 AM, Michael Paquier wrote:
On Fri, Sep 12, 2014 at 7:35 AM, Gaetano Mendola <mend...@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
diff --git a/src/backend/access/gin/gindatapage.c b/src/backend/access/gin/gindatapage.c
index 76e0cb3..e2d15a8 100644
--- a/src/backend/access/gin/gindatapage.c
+++ b/src/backend/access/gin/gindatapage.c
@@ -621,9 +621,9 @@ dataPlaceToPageLeaf(GinBtree btree, Buffer buf, GinBtreeStack *stack,
/*
* Had to split.
*
- * We already divided the segments between the left and the right
- * page. The left page was filled as full as possible, and the rest
- * overflowed to the right page. When building a new index, that's
+ * leafRepackItems already divided the segments between the left and
+ * the right page. It filled the left page as full as possible, and
+ * put the rest to the right page. When building a new index, that's
* good, because the table is scanned from beginning to end and there
* won't be any more insertions to the left page during the build.
* This packs the index as tight as possible. But otherwise, split
@@ -631,9 +631,10 @@ dataPlaceToPageLeaf(GinBtree btree, Buffer buf, GinBtreeStack *stack,
* until they're balanced.
*
* As a further heuristic, when appending items to the end of the
- * page, split 75/25, one the assumption that subsequent insertions
- * will probably also go to the end. This packs the index somewhat
- * tighter when appending to a table, which is very common.
+ * page, try make the left page 75% full, one the assumption that
+ * subsequent insertions will probably also go to the end. This packs
+ * the index somewhat tighter when appending to a table, which is very
+ * common.
*/
if (!btree->isBuild)
{
@@ -645,14 +646,18 @@ dataPlaceToPageLeaf(GinBtree btree, Buffer buf, GinBtreeStack *stack,
if (lastleftinfo->action != GIN_SEGMENT_DELETE)
{
segsize = SizeOfGinPostingList(lastleftinfo->seg);
+
+ /*
+ * Note that we check that the right page doesn't become
+ * more full than the left page even when appending. It's
+ * possible that we added enough items to make both pages
+ * more than 75% full.
+ */
+ if ((leaf->lsize - segsize) - (leaf->rsize + segsize) < 0)
+ break;
if (append)
{
- if ((leaf->lsize - segsize) - (leaf->lsize - segsize) < BLCKSZ / 4)
- break;
- }
- else
- {
- if ((leaf->lsize - segsize) - (leaf->rsize + segsize) < 0)
+ if ((leaf->lsize - segsize) < (BLCKSZ * 3) / 4)
break;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
