Обсуждение: A strange GiST error message or fillfactor of GiST build

Поиск
Список
Период
Сортировка

A strange GiST error message or fillfactor of GiST build

От
Kyotaro HORIGUCHI
Дата:
Hello.

In the discussion about cube's dimention limit [1], I found that
the error messages looks strange.

https://www.postgresql.org/message-id/F0E1A404-A495-4F38-B817-06355B537E88@yandex-team.ru

> postgres=# create table y as  select cube(array(SELECT random() as a FROM generate_series(1,1000))) from
generate_series(1,1e3,1);
 
> SELECT 1000
> postgres=# create index on y using gist(cube );
> ERROR:  index row size 8016 exceeds maximum 8152 for index "y_cube_idx"

This is apparently strange. This is because the message doesn't
count fill factor at the time. It is fixed by passing freespace
to gistSplit() and that allows gistfitpage() to consider
fillfactor as TODO comment within.

After the attached patch applied, the above messages becomes as
follows. (And index can be built being a bit sparse by fill
factor.)

> ERROR:  index row size 8016 exceeds maximum 7333 for index "y_cube_idx"

I'm not sure why 277807bd9e didn't do that completely so I may be
missing something. Is there any thoughts?


There's another issue that ununderstandable message is issued
when (the root) page cannot store two tuples. I'll send a fix for
that sooner if no one objects to check it separately.

> =# create table y as select cube(array(SELECT random() as a FROM generate_series(1,900))) from
generate_series(1,1e3,1);
> =# create index on y using gist (cube);
> ERROR:  failed to add item to index page in "y_cube_idx"

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 8a42effdf7..7773a55552 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -293,7 +293,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
                 memmove(itvec + pos, itvec + pos + 1, sizeof(IndexTuple) * (tlen - pos));
         }
         itvec = gistjoinvector(itvec, &tlen, itup, ntup);
-        dist = gistSplit(rel, page, itvec, tlen, giststate);
+        dist = gistSplit(rel, page, itvec, tlen, freespace, giststate);
 
         /*
          * Check that split didn't produce too many pages.
@@ -1352,6 +1352,7 @@ gistSplit(Relation r,
           Page page,
           IndexTuple *itup,        /* contains compressed entry */
           int len,
+          int freespace,
           GISTSTATE *giststate)
 {
     IndexTuple *lvectup,
@@ -1374,7 +1375,7 @@ gistSplit(Relation r,
         ereport(ERROR,
                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
                  errmsg("index row size %zu exceeds maximum %zu for index \"%s\"",
-                        IndexTupleSize(itup[0]), GiSTPageSize,
+                        IndexTupleSize(itup[0]), GiSTPageSize - freespace,
                         RelationGetRelationName(r))));
 
     memset(v.spl_lisnull, true, sizeof(bool) * giststate->tupdesc->natts);
@@ -1392,9 +1393,10 @@ gistSplit(Relation r,
         rvectup[i] = itup[v.splitVector.spl_right[i] - 1];
 
     /* finalize splitting (may need another split) */
-    if (!gistfitpage(rvectup, v.splitVector.spl_nright))
+    if (!gistfitpage(rvectup, v.splitVector.spl_nright, freespace))
     {
-        res = gistSplit(r, page, rvectup, v.splitVector.spl_nright, giststate);
+        res = gistSplit(r, page, rvectup, v.splitVector.spl_nright, freespace,
+                        giststate);
     }
     else
     {
@@ -1404,12 +1406,13 @@ gistSplit(Relation r,
         res->itup = gistFormTuple(giststate, r, v.spl_rattr, v.spl_risnull, false);
     }
 
-    if (!gistfitpage(lvectup, v.splitVector.spl_nleft))
+    if (!gistfitpage(lvectup, v.splitVector.spl_nleft, freespace))
     {
         SplitedPageLayout *resptr,
                    *subres;
 
-        resptr = subres = gistSplit(r, page, lvectup, v.splitVector.spl_nleft, giststate);
+        resptr = subres = gistSplit(r, page, lvectup, v.splitVector.spl_nleft,
+                                    freespace, giststate);
 
         /* install on list's tail */
         while (resptr->next)
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index dddfe0ae2c..09c7f621bc 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -74,7 +74,7 @@ gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete, Size f
 }
 
 bool
-gistfitpage(IndexTuple *itvec, int len)
+gistfitpage(IndexTuple *itvec, int len, int freespace)
 {
     int            i;
     Size        size = 0;
@@ -82,8 +82,7 @@ gistfitpage(IndexTuple *itvec, int len)
     for (i = 0; i < len; i++)
         size += IndexTupleSize(itvec[i]) + sizeof(ItemIdData);
 
-    /* TODO: Consider fillfactor */
-    return (size <= GiSTPageSize);
+    return (size <= GiSTPageSize - freespace);
 }
 
 /*
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 36ed7244ba..cac3d0b8e5 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -407,7 +407,7 @@ extern bool gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
                 bool markleftchild);
 
 extern SplitedPageLayout *gistSplit(Relation r, Page page, IndexTuple *itup,
-          int len, GISTSTATE *giststate);
+                int len, int freespace, GISTSTATE *giststate);
 
 extern XLogRecPtr gistXLogUpdate(Buffer buffer,
                OffsetNumber *todelete, int ntodelete,
@@ -439,7 +439,7 @@ extern bytea *gistoptions(Datum reloptions, bool validate);
 extern bool gistproperty(Oid index_oid, int attno,
              IndexAMProperty prop, const char *propname,
              bool *res, bool *isnull);
-extern bool gistfitpage(IndexTuple *itvec, int len);
+extern bool gistfitpage(IndexTuple *itvec, int len, int freespace);
 extern bool gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete, Size freespace);
 extern void gistcheckpage(Relation rel, Buffer buf);
 extern Buffer gistNewBuffer(Relation r);

Re: A strange GiST error message or fillfactor of GiST build

От
Andrey Borodin
Дата:
Hi!

> 29 авг. 2018 г., в 5:32, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> написал(а):
>
> Hello.
>
> In the discussion about cube's dimention limit [1], I found that
> the error messages looks strange.
>
> https://www.postgresql.org/message-id/F0E1A404-A495-4F38-B817-06355B537E88@yandex-team.ru
>
>> postgres=# create table y as  select cube(array(SELECT random() as a FROM generate_series(1,1000))) from
generate_series(1,1e3,1); 
>> SELECT 1000
>> postgres=# create index on y using gist(cube );
>> ERROR:  index row size 8016 exceeds maximum 8152 for index "y_cube_idx"
>
> This is apparently strange. This is because the message doesn't
> count fill factor at the time. It is fixed by passing freespace
> to gistSplit() and that allows gistfitpage() to consider
> fillfactor as TODO comment within.
>
> After the attached patch applied, the above messages becomes as
> follows. (And index can be built being a bit sparse by fill
> factor.)

We are passing freespace everywhere. Also, we pass GistInsertState, and GistState.
Maybe let's put GistState into GistInsertState, GistState already has free space, and pass just GistInsertState
everywhere?

Best regards, Andrey Borodin.

Re: A strange GiST error message or fillfactor of GiST build

От
Kyotaro HORIGUCHI
Дата:
Hello.

At Wed, 29 Aug 2018 10:42:59 -0300, Andrey Borodin <x4mmm@yandex-team.ru> wrote in
<6FBE12B2-4F59-4DB9-BDE9-62C8801189A8@yandex-team.ru>
> >> postgres=# create table y as  select cube(array(SELECT random() as a FROM generate_series(1,1000))) from
generate_series(1,1e3,1);
 
> >> SELECT 1000
> >> postgres=# create index on y using gist(cube );
> >> ERROR:  index row size 8016 exceeds maximum 8152 for index "y_cube_idx"
> > 
> > This is apparently strange. This is because the message doesn't
> > count fill factor at the time. It is fixed by passing freespace
> > to gistSplit() and that allows gistfitpage() to consider
> > fillfactor as TODO comment within.
> > 
> > After the attached patch applied, the above messages becomes as
> > follows. (And index can be built being a bit sparse by fill
> > factor.)
> 
> We are passing freespace everywhere. Also, we pass GistInsertState, and GistState.
> Maybe let's put GistState into GistInsertState, GistState already has free space, and pass just GistInsertState
everywhere?

Yeah, I thought something like that first. GISTSTATE doesn't have
freespace size but we could refactor so that all insert-related
routines use GISTInsertState and make GISTBuildState have
it. (patch 1) But this prevents future back-patching so I don't
think this acceptable.

The second patch corresponds to the original patch, which is not
srinked so much.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 5fe611fe9edea2294c53ec9556237e7bf686cb7f Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 30 Aug 2018 14:25:18 +0900
Subject: [PATCH 1/2] Refactor parameter of GiST insertion routines

Many of GiST routeins related to insertion requires Relation,
GISTSTATE and freespace, which is members of GISTInsertState.  This
patch refactors such routines to take one GISTInsertState instead of
taking individual values individually.
---
 src/backend/access/gist/gist.c             | 133 ++++++++++++++---------------
 src/backend/access/gist/gistbuild.c        |  66 +++++++-------
 src/backend/access/gist/gistbuildbuffers.c |  25 +++---
 src/backend/access/gist/gistsplit.c        |  67 ++++++++-------
 src/backend/access/gist/gistutil.c         |  32 ++++---
 src/include/access/gist_private.h          |  43 ++++------
 6 files changed, 177 insertions(+), 189 deletions(-)

diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 8a42effdf7..33b9532bff 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -28,16 +28,15 @@
 
 
 /* non-export function prototypes */
-static void gistfixsplit(GISTInsertState *state, GISTSTATE *giststate);
+static void gistfixsplit(GISTInsertState *state);
 static bool gistinserttuple(GISTInsertState *state, GISTInsertStack *stack,
-                GISTSTATE *giststate, IndexTuple tuple, OffsetNumber oldoffnum);
+                IndexTuple tuple, OffsetNumber oldoffnum);
 static bool gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
-                 GISTSTATE *giststate,
                  IndexTuple *tuples, int ntup, OffsetNumber oldoffnum,
                  Buffer leftchild, Buffer rightchild,
                  bool unlockbuf, bool unlockleftchild);
 static void gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack,
-                GISTSTATE *giststate, List *splitinfo, bool releasebuf);
+                List *splitinfo, bool releasebuf);
 static void gistvacuumpage(Relation rel, Page page, Buffer buffer);
 
 
@@ -152,31 +151,36 @@ gistinsert(Relation r, Datum *values, bool *isnull,
            IndexUniqueCheck checkUnique,
            IndexInfo *indexInfo)
 {
-    GISTSTATE  *giststate = (GISTSTATE *) indexInfo->ii_AmCache;
+    GISTInsertState state;
     IndexTuple    itup;
     MemoryContext oldCxt;
 
+    state.giststate = (GISTSTATE *) indexInfo->ii_AmCache;
+    state.r = r;
+    state.freespace = 0;
+    state.stack = NULL;
+
     /* Initialize GISTSTATE cache if first call in this statement */
-    if (giststate == NULL)
+    if (state.giststate == NULL)
     {
         oldCxt = MemoryContextSwitchTo(indexInfo->ii_Context);
-        giststate = initGISTstate(r);
-        giststate->tempCxt = createTempGistContext();
-        indexInfo->ii_AmCache = (void *) giststate;
+        state.giststate = initGISTstate(r);
+        state.giststate->tempCxt = createTempGistContext();
+        indexInfo->ii_AmCache = (void *) state.giststate;
         MemoryContextSwitchTo(oldCxt);
     }
 
-    oldCxt = MemoryContextSwitchTo(giststate->tempCxt);
+    oldCxt = MemoryContextSwitchTo(state.giststate->tempCxt);
 
-    itup = gistFormTuple(giststate, r,
+    itup = gistFormTuple(&state,
                          values, isnull, true /* size is currently bogus */ );
     itup->t_tid = *ht_ctid;
 
-    gistdoinsert(r, itup, 0, giststate);
+    gistdoinsert(&state, itup);
 
     /* cleanup */
     MemoryContextSwitchTo(oldCxt);
-    MemoryContextReset(giststate->tempCxt);
+    MemoryContextReset(state.giststate->tempCxt);
 
     return false;
 }
@@ -212,7 +216,7 @@ gistinsert(Relation r, Datum *values, bool *isnull,
  * Returns 'true' if the page was split, 'false' otherwise.
  */
 bool
-gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
+gistplacetopage(GISTInsertState *istate,
                 Buffer buffer,
                 IndexTuple *itup, int ntup, OffsetNumber oldoffnum,
                 BlockNumber *newblkno,
@@ -220,6 +224,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
                 List **splitinfo,
                 bool markfollowright)
 {
+    Relation    rel = istate->r;
     BlockNumber blkno = BufferGetBlockNumber(buffer);
     Page        page = BufferGetPage(buffer);
     bool        is_leaf = (GistPageIsLeaf(page)) ? true : false;
@@ -251,7 +256,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
      * one-element todelete array; in the split case, it's handled implicitly
      * because the tuple vector passed to gistSplit won't include this tuple.
      */
-    is_split = gistnospace(page, itup, ntup, oldoffnum, freespace);
+    is_split = gistnospace(page, itup, ntup, oldoffnum, istate->freespace);
 
     /*
      * If leaf page is full, try at first to delete dead tuples. And then
@@ -260,7 +265,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
     if (is_split && GistPageIsLeaf(page) && GistPageHasGarbage(page))
     {
         gistvacuumpage(rel, page, buffer);
-        is_split = gistnospace(page, itup, ntup, oldoffnum, freespace);
+        is_split = gistnospace(page, itup, ntup, oldoffnum, istate->freespace);
     }
 
     if (is_split)
@@ -293,7 +298,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
                 memmove(itvec + pos, itvec + pos + 1, sizeof(IndexTuple) * (tlen - pos));
         }
         itvec = gistjoinvector(itvec, &tlen, itup, ntup);
-        dist = gistSplit(rel, page, itvec, tlen, giststate);
+        dist = gistSplit(istate, page, itvec, tlen);
 
         /*
          * Check that split didn't produce too many pages.
@@ -604,25 +609,20 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
  * so it does not bother releasing palloc'd allocations.
  */
 void
-gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
+gistdoinsert(GISTInsertState *istate, IndexTuple itup)
 {
     ItemId        iid;
     IndexTuple    idxtuple;
     GISTInsertStack firststack;
     GISTInsertStack *stack;
-    GISTInsertState state;
     bool        xlocked = false;
 
-    memset(&state, 0, sizeof(GISTInsertState));
-    state.freespace = freespace;
-    state.r = r;
-
     /* Start from the root */
     firststack.blkno = GIST_ROOT_BLKNO;
     firststack.lsn = 0;
     firststack.parent = NULL;
     firststack.downlinkoffnum = InvalidOffsetNumber;
-    state.stack = stack = &firststack;
+    istate->stack = stack = &firststack;
 
     /*
      * Walk down along the path of smallest penalty, updating the parent
@@ -633,7 +633,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
     for (;;)
     {
         if (XLogRecPtrIsInvalid(stack->lsn))
-            stack->buffer = ReadBuffer(state.r, stack->blkno);
+            stack->buffer = ReadBuffer(istate->r, stack->blkno);
 
         /*
          * Be optimistic and grab shared lock first. Swap it for an exclusive
@@ -642,13 +642,13 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
         if (!xlocked)
         {
             LockBuffer(stack->buffer, GIST_SHARE);
-            gistcheckpage(state.r, stack->buffer);
+            gistcheckpage(istate->r, stack->buffer);
         }
 
         stack->page = (Page) BufferGetPage(stack->buffer);
         stack->lsn = xlocked ?
             PageGetLSN(stack->page) : BufferGetLSNAtomic(stack->buffer);
-        Assert(!RelationNeedsWAL(state.r) || !XLogRecPtrIsInvalid(stack->lsn));
+        Assert(!RelationNeedsWAL(istate->r) || !XLogRecPtrIsInvalid(stack->lsn));
 
         /*
          * If this page was split but the downlink was never inserted to the
@@ -666,11 +666,11 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
                 if (!GistFollowRight(stack->page))
                     continue;
             }
-            gistfixsplit(&state, giststate);
+            gistfixsplit(istate);
 
             UnlockReleaseBuffer(stack->buffer);
             xlocked = false;
-            state.stack = stack = stack->parent;
+            istate->stack = stack = stack->parent;
             continue;
         }
 
@@ -685,7 +685,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
              */
             UnlockReleaseBuffer(stack->buffer);
             xlocked = false;
-            state.stack = stack = stack->parent;
+            istate->stack = stack = stack->parent;
             continue;
         }
 
@@ -700,7 +700,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
             GISTInsertStack *item;
             OffsetNumber downlinkoffnum;
 
-            downlinkoffnum = gistchoose(state.r, stack->page, itup, giststate);
+            downlinkoffnum = gistchoose(istate, stack->page, itup);
             iid = PageGetItemId(stack->page, downlinkoffnum);
             idxtuple = (IndexTuple) PageGetItem(stack->page, iid);
             childblkno = ItemPointerGetBlockNumber(&(idxtuple->t_tid));
@@ -711,7 +711,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
             if (GistTupleIsInvalid(idxtuple))
                 ereport(ERROR,
                         (errmsg("index \"%s\" contains an inner tuple marked as invalid",
-                                RelationGetRelationName(r)),
+                                RelationGetRelationName(istate->r)),
                          errdetail("This is caused by an incomplete page split at crash recovery before upgrading to
PostgreSQL9.1."),
 
                          errhint("Please REINDEX it.")));
 
@@ -719,7 +719,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
              * Check that the key representing the target child node is
              * consistent with the key we're inserting. Update it if it's not.
              */
-            newtup = gistgetadjusted(state.r, idxtuple, itup, giststate);
+            newtup = gistgetadjusted(istate, idxtuple, itup);
             if (newtup)
             {
                 /*
@@ -750,8 +750,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
                  * descend back to the half that's a better fit for the new
                  * tuple.
                  */
-                if (gistinserttuple(&state, stack, giststate, newtup,
-                                    downlinkoffnum))
+                if (gistinserttuple(istate, stack, newtup, downlinkoffnum))
                 {
                     /*
                      * If this was a root split, the root page continues to be
@@ -763,7 +762,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
                     {
                         UnlockReleaseBuffer(stack->buffer);
                         xlocked = false;
-                        state.stack = stack = stack->parent;
+                        istate->stack = stack = stack->parent;
                     }
                     continue;
                 }
@@ -776,7 +775,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
             item->blkno = childblkno;
             item->parent = stack;
             item->downlinkoffnum = downlinkoffnum;
-            state.stack = stack = item;
+            istate->stack = stack = item;
         }
         else
         {
@@ -829,15 +828,14 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
                      */
                     UnlockReleaseBuffer(stack->buffer);
                     xlocked = false;
-                    state.stack = stack = stack->parent;
+                    istate->stack = stack = stack->parent;
                     continue;
                 }
             }
 
             /* now state.stack->(page, buffer and blkno) points to leaf page */
 
-            gistinserttuple(&state, stack, giststate, itup,
-                            InvalidOffsetNumber);
+            gistinserttuple(istate, stack, itup, InvalidOffsetNumber);
             LockBuffer(stack->buffer, GIST_UNLOCK);
 
             /* Release any pins we might still hold before exiting */
@@ -1057,8 +1055,7 @@ gistFindCorrectParent(Relation r, GISTInsertStack *child)
  * Form a downlink pointer for the page in 'buf'.
  */
 static IndexTuple
-gistformdownlink(Relation rel, Buffer buf, GISTSTATE *giststate,
-                 GISTInsertStack *stack)
+gistformdownlink(GISTInsertState *istate, Buffer buf, GISTInsertStack *stack)
 {
     Page        page = BufferGetPage(buf);
     OffsetNumber maxoff;
@@ -1077,8 +1074,7 @@ gistformdownlink(Relation rel, Buffer buf, GISTSTATE *giststate,
         {
             IndexTuple    newdownlink;
 
-            newdownlink = gistgetadjusted(rel, downlink, ituple,
-                                          giststate);
+            newdownlink = gistgetadjusted(istate, downlink, ituple);
             if (newdownlink)
                 downlink = newdownlink;
         }
@@ -1099,7 +1095,7 @@ gistformdownlink(Relation rel, Buffer buf, GISTSTATE *giststate,
         ItemId        iid;
 
         LockBuffer(stack->parent->buffer, GIST_EXCLUSIVE);
-        gistFindCorrectParent(rel, stack);
+        gistFindCorrectParent(istate->r, stack);
         iid = PageGetItemId(stack->parent->page, stack->downlinkoffnum);
         downlink = (IndexTuple) PageGetItem(stack->parent->page, iid);
         downlink = CopyIndexTuple(downlink);
@@ -1117,7 +1113,7 @@ gistformdownlink(Relation rel, Buffer buf, GISTSTATE *giststate,
  * Complete the incomplete split of state->stack->page.
  */
 static void
-gistfixsplit(GISTInsertState *state, GISTSTATE *giststate)
+gistfixsplit(GISTInsertState *state)
 {
     GISTInsertStack *stack = state->stack;
     Buffer        buf;
@@ -1144,7 +1140,7 @@ gistfixsplit(GISTInsertState *state, GISTSTATE *giststate)
         page = BufferGetPage(buf);
 
         /* Form the new downlink tuples to insert to parent */
-        downlink = gistformdownlink(state->r, buf, giststate, stack);
+        downlink = gistformdownlink(state, buf, stack);
 
         si->buf = buf;
         si->downlink = downlink;
@@ -1162,7 +1158,7 @@ gistfixsplit(GISTInsertState *state, GISTSTATE *giststate)
     }
 
     /* Insert the downlinks */
-    gistfinishsplit(state, stack, giststate, splitinfo, false);
+    gistfinishsplit(state, stack, splitinfo, false);
 }
 
 /*
@@ -1177,9 +1173,9 @@ gistfixsplit(GISTInsertState *state, GISTSTATE *giststate)
  */
 static bool
 gistinserttuple(GISTInsertState *state, GISTInsertStack *stack,
-                GISTSTATE *giststate, IndexTuple tuple, OffsetNumber oldoffnum)
+                IndexTuple tuple, OffsetNumber oldoffnum)
 {
-    return gistinserttuples(state, stack, giststate, &tuple, 1, oldoffnum,
+    return gistinserttuples(state, stack, &tuple, 1, oldoffnum,
                             InvalidBuffer, InvalidBuffer, false, false);
 }
 
@@ -1211,7 +1207,6 @@ gistinserttuple(GISTInsertState *state, GISTInsertStack *stack,
  */
 static bool
 gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
-                 GISTSTATE *giststate,
                  IndexTuple *tuples, int ntup, OffsetNumber oldoffnum,
                  Buffer leftchild, Buffer rightchild,
                  bool unlockbuf, bool unlockleftchild)
@@ -1226,13 +1221,8 @@ gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
     CheckForSerializableConflictIn(state->r, NULL, stack->buffer);
 
     /* Insert the tuple(s) to the page, splitting the page if necessary */
-    is_split = gistplacetopage(state->r, state->freespace, giststate,
-                               stack->buffer,
-                               tuples, ntup,
-                               oldoffnum, NULL,
-                               leftchild,
-                               &splitinfo,
-                               true);
+    is_split = gistplacetopage(state, stack->buffer, tuples, ntup,
+                               oldoffnum, NULL, leftchild, &splitinfo, true);
 
     /*
      * Before recursing up in case the page was split, release locks on the
@@ -1251,7 +1241,7 @@ gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
      * didn't have to split, release it ourselves.
      */
     if (splitinfo)
-        gistfinishsplit(state, stack, giststate, splitinfo, unlockbuf);
+        gistfinishsplit(state, stack, splitinfo, unlockbuf);
     else if (unlockbuf)
         LockBuffer(stack->buffer, GIST_UNLOCK);
 
@@ -1269,7 +1259,7 @@ gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
  */
 static void
 gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack,
-                GISTSTATE *giststate, List *splitinfo, bool unlockbuf)
+                List *splitinfo, bool unlockbuf)
 {
     ListCell   *lc;
     List       *reversed;
@@ -1307,7 +1297,7 @@ gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack,
         right = (GISTPageSplitInfo *) linitial(reversed);
         left = (GISTPageSplitInfo *) lsecond(reversed);
 
-        if (gistinserttuples(state, stack->parent, giststate,
+        if (gistinserttuples(state, stack->parent,
                              &right->downlink, 1,
                              InvalidOffsetNumber,
                              left->buf, right->buf, false, false))
@@ -1332,7 +1322,7 @@ gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack,
      */
     tuples[0] = left->downlink;
     tuples[1] = right->downlink;
-    gistinserttuples(state, stack->parent, giststate,
+    gistinserttuples(state, stack->parent,
                      tuples, 2,
                      stack->downlinkoffnum,
                      left->buf, right->buf,
@@ -1348,12 +1338,12 @@ gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack,
  * it will split page until keys will fit in every page.
  */
 SplitedPageLayout *
-gistSplit(Relation r,
+gistSplit(GISTInsertState *istate,
           Page page,
           IndexTuple *itup,        /* contains compressed entry */
-          int len,
-          GISTSTATE *giststate)
+          int len)
 {
+    GISTSTATE  *giststate = istate->giststate;
     IndexTuple *lvectup,
                *rvectup;
     GistSplitVector v;
@@ -1375,11 +1365,11 @@ gistSplit(Relation r,
                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
                  errmsg("index row size %zu exceeds maximum %zu for index \"%s\"",
                         IndexTupleSize(itup[0]), GiSTPageSize,
-                        RelationGetRelationName(r))));
+                        RelationGetRelationName(istate->r))));
 
     memset(v.spl_lisnull, true, sizeof(bool) * giststate->tupdesc->natts);
     memset(v.spl_risnull, true, sizeof(bool) * giststate->tupdesc->natts);
-    gistSplitByKey(r, page, itup, len, giststate, &v, 0);
+    gistSplitByKey(istate, page, itup, len, &v, 0);
 
     /* form left and right vector */
     lvectup = (IndexTuple *) palloc(sizeof(IndexTuple) * (len + 1));
@@ -1394,14 +1384,14 @@ gistSplit(Relation r,
     /* finalize splitting (may need another split) */
     if (!gistfitpage(rvectup, v.splitVector.spl_nright))
     {
-        res = gistSplit(r, page, rvectup, v.splitVector.spl_nright, giststate);
+        res = gistSplit(istate, page, rvectup, v.splitVector.spl_nright);
     }
     else
     {
         ROTATEDIST(res);
         res->block.num = v.splitVector.spl_nright;
         res->list = gistfillitupvec(rvectup, v.splitVector.spl_nright, &(res->lenlist));
-        res->itup = gistFormTuple(giststate, r, v.spl_rattr, v.spl_risnull, false);
+        res->itup = gistFormTuple(istate, v.spl_rattr, v.spl_risnull, false);
     }
 
     if (!gistfitpage(lvectup, v.splitVector.spl_nleft))
@@ -1409,7 +1399,8 @@ gistSplit(Relation r,
         SplitedPageLayout *resptr,
                    *subres;
 
-        resptr = subres = gistSplit(r, page, lvectup, v.splitVector.spl_nleft, giststate);
+        resptr = subres = gistSplit(istate, page,
+                                    lvectup, v.splitVector.spl_nleft);
 
         /* install on list's tail */
         while (resptr->next)
@@ -1423,7 +1414,7 @@ gistSplit(Relation r,
         ROTATEDIST(res);
         res->block.num = v.splitVector.spl_nleft;
         res->list = gistfillitupvec(lvectup, v.splitVector.spl_nleft, &(res->lenlist));
-        res->itup = gistFormTuple(giststate, r, v.spl_lattr, v.spl_lisnull, false);
+        res->itup = gistFormTuple(istate, v.spl_lattr, v.spl_lisnull, false);
     }
 
     return res;
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 434f15f014..cc22e0a71a 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -55,14 +55,11 @@ typedef enum
 /* Working state for gistbuild and its callback */
 typedef struct
 {
-    Relation    indexrel;
-    GISTSTATE  *giststate;
+    GISTInsertState insertstate;
 
     int64        indtuples;        /* number of tuples indexed */
     int64        indtuplesSize;    /* total size of all indexed tuples */
 
-    Size        freespace;        /* amount of free space to leave on pages */
-
     /*
      * Extra data structures used during a buffering build. 'gfbb' contains
      * information related to managing the build buffers. 'parentMap' is a
@@ -121,7 +118,7 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
     MemoryContext oldcxt = CurrentMemoryContext;
     int            fillfactor;
 
-    buildstate.indexrel = index;
+    buildstate.insertstate.r = index;
     if (index->rd_options)
     {
         /* Get buffering mode from the options string */
@@ -147,7 +144,9 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
         fillfactor = GIST_DEFAULT_FILLFACTOR;
     }
     /* Calculate target amount of free space to leave on pages */
-    buildstate.freespace = BLCKSZ * (100 - fillfactor) / 100;
+    buildstate.insertstate.freespace = BLCKSZ * (100 - fillfactor) / 100;
+
+    buildstate.insertstate.stack = NULL;
 
     /*
      * We expect to be called exactly once for any index relation. If that's
@@ -158,14 +157,14 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
              RelationGetRelationName(index));
 
     /* no locking is needed */
-    buildstate.giststate = initGISTstate(index);
+    buildstate.insertstate.giststate = initGISTstate(index);
 
     /*
      * Create a temporary memory context that is reset once for each tuple
      * processed.  (Note: we don't bother to make this a child of the
      * giststate's scanCxt, so we have to delete it separately at the end.)
      */
-    buildstate.giststate->tempCxt = createTempGistContext();
+    buildstate.insertstate.giststate->tempCxt = createTempGistContext();
 
     /* initialize the root page */
     buffer = gistNewBuffer(index);
@@ -218,9 +217,9 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 
     /* okay, all heap tuples are indexed */
     MemoryContextSwitchTo(oldcxt);
-    MemoryContextDelete(buildstate.giststate->tempCxt);
+    MemoryContextDelete(buildstate.insertstate.giststate->tempCxt);
 
-    freeGISTstate(buildstate.giststate);
+    freeGISTstate(buildstate.insertstate.giststate);
 
     /*
      * Return statistics
@@ -263,7 +262,7 @@ gistValidateBufferingOption(const char *value)
 static void
 gistInitBuffering(GISTBuildState *buildstate)
 {
-    Relation    index = buildstate->indexrel;
+    Relation    index = buildstate->insertstate.r;
     int            pagesPerBuffer;
     Size        pageFreeSpace;
     Size        itupAvgSize,
@@ -276,7 +275,7 @@ gistInitBuffering(GISTBuildState *buildstate)
     /* Calc space of index page which is available for index tuples */
     pageFreeSpace = BLCKSZ - SizeOfPageHeaderData - sizeof(GISTPageOpaqueData)
         - sizeof(ItemIdData)
-        - buildstate->freespace;
+        - buildstate->insertstate.freespace;
 
     /*
      * Calculate average size of already inserted index tuples using gathered
@@ -432,7 +431,7 @@ calculatePagesPerBuffer(GISTBuildState *buildstate, int levelStep)
     /* Calc space of index page which is available for index tuples */
     pageFreeSpace = BLCKSZ - SizeOfPageHeaderData - sizeof(GISTPageOpaqueData)
         - sizeof(ItemIdData)
-        - buildstate->freespace;
+        - buildstate->insertstate.freespace;
 
     /*
      * Calculate average size of already inserted index tuples using gathered
@@ -466,10 +465,10 @@ gistBuildCallback(Relation index,
     IndexTuple    itup;
     MemoryContext oldCtx;
 
-    oldCtx = MemoryContextSwitchTo(buildstate->giststate->tempCxt);
+    oldCtx = MemoryContextSwitchTo(buildstate->insertstate.giststate->tempCxt);
 
     /* form an index tuple and point it at the heap tuple */
-    itup = gistFormTuple(buildstate->giststate, index, values, isnull, true);
+    itup = gistFormTuple(&buildstate->insertstate, values, isnull, true);
     itup->t_tid = htup->t_self;
 
     if (buildstate->bufferingMode == GIST_BUFFERING_ACTIVE)
@@ -483,8 +482,7 @@ gistBuildCallback(Relation index,
          * There's no buffers (yet). Since we already have the index relation
          * locked, we call gistdoinsert directly.
          */
-        gistdoinsert(index, itup, buildstate->freespace,
-                     buildstate->giststate);
+        gistdoinsert(&buildstate->insertstate, itup);
     }
 
     /* Update tuple count and total size. */
@@ -492,7 +490,7 @@ gistBuildCallback(Relation index,
     buildstate->indtuplesSize += IndexTupleSize(itup);
 
     MemoryContextSwitchTo(oldCtx);
-    MemoryContextReset(buildstate->giststate->tempCxt);
+    MemoryContextReset(buildstate->insertstate.giststate->tempCxt);
 
     if (buildstate->bufferingMode == GIST_BUFFERING_ACTIVE &&
         buildstate->indtuples % BUFFERING_MODE_TUPLE_SIZE_STATS_TARGET == 0)
@@ -546,9 +544,10 @@ static bool
 gistProcessItup(GISTBuildState *buildstate, IndexTuple itup,
                 BlockNumber startblkno, int startlevel)
 {
-    GISTSTATE  *giststate = buildstate->giststate;
+    GISTInsertState *istate = &buildstate->insertstate;
+    Relation    indexrel = istate->r;
+    GISTSTATE  *giststate = istate->giststate;
     GISTBuildBuffers *gfbb = buildstate->gfbb;
-    Relation    indexrel = buildstate->indexrel;
     BlockNumber childblkno;
     Buffer        buffer;
     bool        result = false;
@@ -591,7 +590,7 @@ gistProcessItup(GISTBuildState *buildstate, IndexTuple itup,
         LockBuffer(buffer, GIST_EXCLUSIVE);
 
         page = (Page) BufferGetPage(buffer);
-        childoffnum = gistchoose(indexrel, page, itup, giststate);
+        childoffnum = gistchoose(istate, page, itup);
         iid = PageGetItemId(page, childoffnum);
         idxtuple = (IndexTuple) PageGetItem(page, iid);
         childblkno = ItemPointerGetBlockNumber(&(idxtuple->t_tid));
@@ -603,7 +602,7 @@ gistProcessItup(GISTBuildState *buildstate, IndexTuple itup,
          * Check that the key representing the target child node is consistent
          * with the key we're inserting. Update it if it's not.
          */
-        newtup = gistgetadjusted(indexrel, idxtuple, itup, giststate);
+        newtup = gistgetadjusted(istate, idxtuple, itup);
         if (newtup)
         {
             blkno = gistbufferinginserttuples(buildstate,
@@ -683,9 +682,7 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
     bool        is_split;
     BlockNumber placed_to_blk = InvalidBlockNumber;
 
-    is_split = gistplacetopage(buildstate->indexrel,
-                               buildstate->freespace,
-                               buildstate->giststate,
+    is_split = gistplacetopage(&buildstate->insertstate,
                                buffer,
                                itup, ntup, oldoffnum, &placed_to_blk,
                                InvalidBuffer,
@@ -722,7 +719,7 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
                 ItemId        iid = PageGetItemId(page, off);
                 IndexTuple    idxtuple = (IndexTuple) PageGetItem(page, iid);
                 BlockNumber childblkno = ItemPointerGetBlockNumber(&(idxtuple->t_tid));
-                Buffer        childbuf = ReadBuffer(buildstate->indexrel, childblkno);
+                Buffer        childbuf = ReadBuffer(buildstate->insertstate.r, childblkno);
 
                 LockBuffer(childbuf, GIST_SHARE);
                 gistMemorizeAllDownlinks(buildstate, childbuf);
@@ -766,11 +763,8 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
          * with the tuples already on the pages, but also the tuples in the
          * buffers that will eventually be inserted to them.
          */
-        gistRelocateBuildBuffersOnSplit(gfbb,
-                                        buildstate->giststate,
-                                        buildstate->indexrel,
-                                        level,
-                                        buffer, splitinfo);
+        gistRelocateBuildBuffersOnSplit(&buildstate->insertstate,
+                                        gfbb, level, buffer, splitinfo);
 
         /* Create an array of all the downlink tuples */
         ndownlinks = list_length(splitinfo);
@@ -866,10 +860,10 @@ gistBufferingFindCorrectParent(GISTBuildState *buildstate,
         parent = *parentblkno;
     }
 
-    buffer = ReadBuffer(buildstate->indexrel, parent);
+    buffer = ReadBuffer(buildstate->insertstate.r, parent);
     page = BufferGetPage(buffer);
     LockBuffer(buffer, GIST_EXCLUSIVE);
-    gistcheckpage(buildstate->indexrel, buffer);
+    gistcheckpage(buildstate->insertstate.r, buffer);
     maxoff = PageGetMaxOffsetNumber(page);
 
     /* Check if it was not moved */
@@ -976,7 +970,7 @@ gistProcessEmptyingQueue(GISTBuildState *buildstate)
             }
 
             /* Free all the memory allocated during index tuple processing */
-            MemoryContextReset(buildstate->giststate->tempCxt);
+            MemoryContextReset(buildstate->insertstate.giststate->tempCxt);
         }
     }
 }
@@ -995,7 +989,7 @@ gistEmptyAllBuffers(GISTBuildState *buildstate)
     MemoryContext oldCtx;
     int            i;
 
-    oldCtx = MemoryContextSwitchTo(buildstate->giststate->tempCxt);
+    oldCtx = MemoryContextSwitchTo(buildstate->insertstate.giststate->tempCxt);
 
     /*
      * Iterate through the levels from top to bottom.
@@ -1027,7 +1021,7 @@ gistEmptyAllBuffers(GISTBuildState *buildstate)
                     nodeBuffer->queuedForEmptying = true;
                     gfbb->bufferEmptyingQueue =
                         lcons(nodeBuffer, gfbb->bufferEmptyingQueue);
-                    MemoryContextSwitchTo(buildstate->giststate->tempCxt);
+                    MemoryContextSwitchTo(buildstate->insertstate.giststate->tempCxt);
                 }
                 gistProcessEmptyingQueue(buildstate);
             }
diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c
index 97033983e3..b06de8a283 100644
--- a/src/backend/access/gist/gistbuildbuffers.c
+++ b/src/backend/access/gist/gistbuildbuffers.c
@@ -534,9 +534,8 @@ typedef struct
  * in 'splitinfo' to include the tuples in the buffers.
  */
 void
-gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
-                                Relation r, int level,
-                                Buffer buffer, List *splitinfo)
+gistRelocateBuildBuffersOnSplit(GISTInsertState *istate, GISTBuildBuffers *gfbb,
+                                int level, Buffer buffer, List *splitinfo)
 {
     RelocationBufferInfo *relocationBuffersInfos;
     bool        found;
@@ -602,7 +601,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
         GISTNodeBuffer *newNodeBuffer;
 
         /* Decompress parent index tuple of node buffer page. */
-        gistDeCompressAtt(giststate, r,
+        gistDeCompressAtt(istate,
                           si->downlink, NULL, (OffsetNumber) 0,
                           relocationBuffersInfos[i].entry,
                           relocationBuffersInfos[i].isnull);
@@ -614,7 +613,8 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
          * were relinked to the temporary buffer, so the original one is now
          * empty.
          */
-        newNodeBuffer = gistGetNodeBuffer(gfbb, giststate, BufferGetBlockNumber(si->buf), level);
+        newNodeBuffer = gistGetNodeBuffer(gfbb, istate->giststate,
+                                          BufferGetBlockNumber(si->buf), level);
 
         relocationBuffersInfos[i].nodeBuffer = newNodeBuffer;
         relocationBuffersInfos[i].splitinfo = si;
@@ -639,8 +639,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
         IndexTuple    newtup;
         RelocationBufferInfo *targetBufferInfo;
 
-        gistDeCompressAtt(giststate, r,
-                          itup, NULL, (OffsetNumber) 0, entry, isnull);
+        gistDeCompressAtt(istate, itup, NULL, (OffsetNumber) 0, entry, isnull);
 
         /* default to using first page (shouldn't matter) */
         which = 0;
@@ -665,12 +664,12 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
             zero_penalty = true;
 
             /* Loop over index attributes. */
-            for (j = 0; j < r->rd_att->natts; j++)
+            for (j = 0; j < istate->r->rd_att->natts; j++)
             {
                 float        usize;
 
                 /* Compute penalty for this column. */
-                usize = gistpenalty(giststate, j,
+                usize = gistpenalty(istate->giststate, j,
                                     &splitPageInfo->entry[j],
                                     splitPageInfo->isnull[j],
                                     &entry[j], isnull[j]);
@@ -691,7 +690,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
                     which = i;
                     best_penalty[j] = usize;
 
-                    if (j < r->rd_att->natts - 1)
+                    if (j < istate->r->rd_att->natts - 1)
                         best_penalty[j + 1] = -1;
                 }
                 else if (best_penalty[j] == usize)
@@ -730,11 +729,11 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
         gistPushItupToNodeBuffer(gfbb, targetBufferInfo->nodeBuffer, itup);
 
         /* Adjust the downlink for this page, if needed. */
-        newtup = gistgetadjusted(r, targetBufferInfo->splitinfo->downlink,
-                                 itup, giststate);
+        newtup = gistgetadjusted(istate,
+                                 targetBufferInfo->splitinfo->downlink, itup);
         if (newtup)
         {
-            gistDeCompressAtt(giststate, r,
+            gistDeCompressAtt(istate,
                               newtup, NULL, (OffsetNumber) 0,
                               targetBufferInfo->entry,
                               targetBufferInfo->isnull);
diff --git a/src/backend/access/gist/gistsplit.c b/src/backend/access/gist/gistsplit.c
index a7038cca67..6c1b493a64 100644
--- a/src/backend/access/gist/gistsplit.c
+++ b/src/backend/access/gist/gistsplit.c
@@ -110,7 +110,7 @@ gistunionsubkey(GISTSTATE *giststate, IndexTuple *itvec, GistSplitVector *spl)
  * Returns number of don't-cares found.
  */
 static int
-findDontCares(Relation r, GISTSTATE *giststate, GISTENTRY *valvec,
+findDontCares(GISTInsertState *istate, GISTENTRY *valvec,
               GistSplitVector *spl, int attno)
 {
     int            i;
@@ -124,13 +124,13 @@ findDontCares(Relation r, GISTSTATE *giststate, GISTENTRY *valvec,
      * attno column is known all-not-null (see gistSplitByKey), so we need not
      * check for nulls
      */
-    gistentryinit(entry, spl->splitVector.spl_rdatum, r, NULL,
+    gistentryinit(entry, spl->splitVector.spl_rdatum, istate->r, NULL,
                   (OffsetNumber) 0, false);
     for (i = 0; i < spl->splitVector.spl_nleft; i++)
     {
         int            j = spl->splitVector.spl_left[i];
-        float        penalty = gistpenalty(giststate, attno, &entry, false,
-                                          &valvec[j], false);
+        float        penalty = gistpenalty(istate->giststate, attno, &entry,
+                                          false, &valvec[j], false);
 
         if (penalty == 0.0)
         {
@@ -140,13 +140,13 @@ findDontCares(Relation r, GISTSTATE *giststate, GISTENTRY *valvec,
     }
 
     /* And conversely for the right-side tuples */
-    gistentryinit(entry, spl->splitVector.spl_ldatum, r, NULL,
+    gistentryinit(entry, spl->splitVector.spl_ldatum, istate->r, NULL,
                   (OffsetNumber) 0, false);
     for (i = 0; i < spl->splitVector.spl_nright; i++)
     {
         int            j = spl->splitVector.spl_right[i];
-        float        penalty = gistpenalty(giststate, attno, &entry, false,
-                                          &valvec[j], false);
+        float        penalty = gistpenalty(istate->giststate, attno, &entry,
+                                          false, &valvec[j], false);
 
         if (penalty == 0.0)
         {
@@ -197,15 +197,16 @@ removeDontCares(OffsetNumber *a, int *len, const bool *dontcare)
  * at attno.
  */
 static void
-placeOne(Relation r, GISTSTATE *giststate, GistSplitVector *v,
+placeOne(GISTInsertState *istate, GistSplitVector *v,
          IndexTuple itup, OffsetNumber off, int attno)
 {
+    Relation    r = istate->r;
+    GISTSTATE  *giststate = istate->giststate;
     GISTENTRY    identry[INDEX_MAX_KEYS];
     bool        isnull[INDEX_MAX_KEYS];
     bool        toLeft = true;
 
-    gistDeCompressAtt(giststate, r, itup, NULL, (OffsetNumber) 0,
-                      identry, isnull);
+    gistDeCompressAtt(istate, itup, NULL, (OffsetNumber) 0, identry, isnull);
 
     for (; attno < giststate->tupdesc->natts; attno++)
     {
@@ -255,9 +256,11 @@ do {    \
  * PickSplit method didn't do so.
  */
 static void
-supportSecondarySplit(Relation r, GISTSTATE *giststate, int attno,
+supportSecondarySplit(GISTInsertState *istate, int attno,
                       GIST_SPLITVEC *sv, Datum oldL, Datum oldR)
 {
+    Relation    r = istate->r;
+    GISTSTATE  *giststate = istate->giststate;
     bool        leaveOnLeft = true,
                 tmpBool;
     GISTENTRY    entryL,
@@ -412,9 +415,11 @@ genericPickSplit(GISTSTATE *giststate, GistEntryVector *entryvec, GIST_SPLITVEC
  * A true result implies there is at least one more index column.
  */
 static bool
-gistUserPicksplit(Relation r, GistEntryVector *entryvec, int attno, GistSplitVector *v,
-                  IndexTuple *itup, int len, GISTSTATE *giststate)
+gistUserPicksplit(GISTInsertState *istate, GistEntryVector *entryvec, int attno,
+                  GistSplitVector *v, IndexTuple *itup, int len)
 {
+    Relation        r = istate->r;
+    GISTSTATE       *giststate = istate->giststate;
     GIST_SPLITVEC *sv = &v->splitVector;
 
     /*
@@ -470,7 +475,7 @@ gistUserPicksplit(Relation r, GistEntryVector *entryvec, int attno, GistSplitVec
 
     /* Clean up if PickSplit didn't take care of a secondary split */
     if (sv->spl_ldatum_exists || sv->spl_rdatum_exists)
-        supportSecondarySplit(r, giststate, attno, sv,
+        supportSecondarySplit(istate, attno, sv,
                               v->spl_lattr[attno], v->spl_rattr[attno]);
 
     /* emit union datums computed by PickSplit back to v arrays */
@@ -503,7 +508,7 @@ gistUserPicksplit(Relation r, GistEntryVector *entryvec, int attno, GistSplitVec
          */
         v->spl_dontcare = (bool *) palloc0(sizeof(bool) * (entryvec->n + 1));
 
-        NumDontCare = findDontCares(r, giststate, entryvec->vector, v, attno);
+        NumDontCare = findDontCares(istate, entryvec->vector, v, attno);
 
         if (NumDontCare > 0)
         {
@@ -562,7 +567,7 @@ gistUserPicksplit(Relation r, GistEntryVector *entryvec, int attno, GistSplitVec
                 Assert(toMove < entryvec->n);
 
                 /* ... and assign it to cheaper side */
-                placeOne(r, giststate, v, itup[toMove - 1], toMove, attno + 1);
+                placeOne(istate, v, itup[toMove - 1], toMove, attno + 1);
 
                 /*
                  * At this point the union keys are wrong, but we don't care
@@ -601,11 +606,10 @@ gistSplitHalf(GIST_SPLITVEC *v, int len)
 /*
  * gistSplitByKey: main entry point for page-splitting algorithm
  *
- * r: index relation
+ * istate: GIST insert state
  * page: page being split
  * itup: array of IndexTuples to be processed
  * len: number of IndexTuples to be processed (must be at least 2)
- * giststate: additional info about index
  * v: working state and output area
  * attno: column we are working on (zero-based index)
  *
@@ -620,10 +624,12 @@ gistSplitHalf(GIST_SPLITVEC *v, int len)
  * recurse to the next column by passing attno+1.
  */
 void
-gistSplitByKey(Relation r, Page page, IndexTuple *itup, int len,
-               GISTSTATE *giststate, GistSplitVector *v, int attno)
+gistSplitByKey(GISTInsertState *istate, Page page, IndexTuple *itup, int len,
+               GistSplitVector *v, int attno)
 {
     GistEntryVector *entryvec;
+    GISTSTATE     *giststate = istate->giststate;
+    TupleDesc      tupdesc = giststate->tupdesc;
     OffsetNumber *offNullTuples;
     int            nOffNullTuples = 0;
     int            i;
@@ -639,10 +645,9 @@ gistSplitByKey(Relation r, Page page, IndexTuple *itup, int len,
         Datum        datum;
         bool        IsNull;
 
-        datum = index_getattr(itup[i - 1], attno + 1, giststate->tupdesc,
-                              &IsNull);
+        datum = index_getattr(itup[i - 1], attno + 1, tupdesc, &IsNull);
         gistdentryinit(giststate, attno, &(entryvec->vector[i]),
-                       datum, r, page, i,
+                       datum, istate->r, page, i,
                        false, IsNull);
         if (IsNull)
             offNullTuples[nOffNullTuples++] = i;
@@ -657,8 +662,8 @@ gistSplitByKey(Relation r, Page page, IndexTuple *itup, int len,
          */
         v->spl_risnull[attno] = v->spl_lisnull[attno] = true;
 
-        if (attno + 1 < giststate->tupdesc->natts)
-            gistSplitByKey(r, page, itup, len, giststate, v, attno + 1);
+        if (attno + 1 < tupdesc->natts)
+            gistSplitByKey(istate, page, itup, len, v, attno + 1);
         else
             gistSplitHalf(&v->splitVector, len);
     }
@@ -683,7 +688,7 @@ gistSplitByKey(Relation r, Page page, IndexTuple *itup, int len,
                 v->splitVector.spl_left[v->splitVector.spl_nleft++] = i;
 
         /* Compute union keys, unless outer recursion level will handle it */
-        if (attno == 0 && giststate->tupdesc->natts == 1)
+        if (attno == 0 && tupdesc->natts == 1)
         {
             v->spl_dontcare = NULL;
             gistunionsubkey(giststate, itup, v);
@@ -694,13 +699,13 @@ gistSplitByKey(Relation r, Page page, IndexTuple *itup, int len,
         /*
          * All keys are not-null, so apply user-defined PickSplit method
          */
-        if (gistUserPicksplit(r, entryvec, attno, v, itup, len, giststate))
+        if (gistUserPicksplit(istate, entryvec, attno, v, itup, len))
         {
             /*
              * Splitting on attno column is not optimal, so consider
              * redistributing don't-care tuples according to the next column
              */
-            Assert(attno + 1 < giststate->tupdesc->natts);
+            Assert(attno + 1 < tupdesc->natts);
 
             if (v->spl_dontcare == NULL)
             {
@@ -708,7 +713,7 @@ gistSplitByKey(Relation r, Page page, IndexTuple *itup, int len,
                  * This split was actually degenerate, so ignore it altogether
                  * and just split according to the next column.
                  */
-                gistSplitByKey(r, page, itup, len, giststate, v, attno + 1);
+                gistSplitByKey(istate, page, itup, len, v, attno + 1);
             }
             else
             {
@@ -744,7 +749,7 @@ gistSplitByKey(Relation r, Page page, IndexTuple *itup, int len,
                 memcpy(backupSplit.spl_right, v->splitVector.spl_right, sizeof(OffsetNumber) *
v->splitVector.spl_nright);
 
                 /* Recursively decide how to split the don't-care tuples */
-                gistSplitByKey(r, page, newitup, newlen, giststate, v, attno + 1);
+                gistSplitByKey(istate, page, newitup, newlen, v, attno + 1);
 
                 /* Merge result of subsplit with non-don't-care tuples */
                 for (i = 0; i < v->splitVector.spl_nleft; i++)
@@ -771,7 +776,7 @@ gistSplitByKey(Relation r, Page page, IndexTuple *itup, int len,
      * that PickSplit (or the special cases above) produced correct union
      * datums.
      */
-    if (attno == 0 && giststate->tupdesc->natts > 1)
+    if (attno == 0 && tupdesc->natts > 1)
     {
         v->spl_dontcare = NULL;
         gistunionsubkey(giststate, itup, v);
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index dddfe0ae2c..20802f1adc 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -213,14 +213,14 @@ gistMakeUnionItVec(GISTSTATE *giststate, IndexTuple *itvec, int len,
  * method to the specified IndexTuple vector.
  */
 IndexTuple
-gistunion(Relation r, IndexTuple *itvec, int len, GISTSTATE *giststate)
+gistunion(GISTInsertState *istate, IndexTuple *itvec, int len)
 {
     Datum        attr[INDEX_MAX_KEYS];
     bool        isnull[INDEX_MAX_KEYS];
 
-    gistMakeUnionItVec(giststate, itvec, len, attr, isnull);
+    gistMakeUnionItVec(istate->giststate, itvec, len, attr, isnull);
 
-    return gistFormTuple(giststate, r, attr, isnull, false);
+    return gistFormTuple(istate, attr, isnull, false);
 }
 
 /*
@@ -290,9 +290,11 @@ gistKeyIsEQ(GISTSTATE *giststate, int attno, Datum a, Datum b)
  * Decompress all keys in tuple
  */
 void
-gistDeCompressAtt(GISTSTATE *giststate, Relation r, IndexTuple tuple, Page p,
+gistDeCompressAtt(GISTInsertState *istate, IndexTuple tuple, Page p,
                   OffsetNumber o, GISTENTRY *attdata, bool *isnull)
 {
+    Relation    r = istate->r;
+    GISTSTATE  *giststate = istate->giststate;
     int            i;
 
     for (i = 0; i < r->rd_att->natts; i++)
@@ -310,8 +312,10 @@ gistDeCompressAtt(GISTSTATE *giststate, Relation r, IndexTuple tuple, Page p,
  * Forms union of oldtup and addtup, if union == oldtup then return NULL
  */
 IndexTuple
-gistgetadjusted(Relation r, IndexTuple oldtup, IndexTuple addtup, GISTSTATE *giststate)
+gistgetadjusted(GISTInsertState *istate, IndexTuple oldtup, IndexTuple addtup)
 {
+    Relation    r = istate->r;
+    GISTSTATE  *giststate = istate->giststate;
     bool        neednew = false;
     GISTENTRY    oldentries[INDEX_MAX_KEYS],
                 addentries[INDEX_MAX_KEYS];
@@ -322,10 +326,10 @@ gistgetadjusted(Relation r, IndexTuple oldtup, IndexTuple addtup, GISTSTATE *gis
     IndexTuple    newtup = NULL;
     int            i;
 
-    gistDeCompressAtt(giststate, r, oldtup, NULL,
+    gistDeCompressAtt(istate, oldtup, NULL,
                       (OffsetNumber) 0, oldentries, oldisnull);
 
-    gistDeCompressAtt(giststate, r, addtup, NULL,
+    gistDeCompressAtt(istate, addtup, NULL,
                       (OffsetNumber) 0, addentries, addisnull);
 
     for (i = 0; i < r->rd_att->natts; i++)
@@ -354,7 +358,7 @@ gistgetadjusted(Relation r, IndexTuple oldtup, IndexTuple addtup, GISTSTATE *gis
     if (neednew)
     {
         /* need to update key */
-        newtup = gistFormTuple(giststate, r, attr, isnull, false);
+        newtup = gistFormTuple(istate, attr, isnull, false);
         newtup->t_tid = oldtup->t_tid;
     }
 
@@ -368,9 +372,11 @@ gistgetadjusted(Relation r, IndexTuple oldtup, IndexTuple addtup, GISTSTATE *gis
  * Returns the index of the page entry to insert into.
  */
 OffsetNumber
-gistchoose(Relation r, Page p, IndexTuple it,    /* it has compressed entry */
-           GISTSTATE *giststate)
+gistchoose(GISTInsertState *istate, Page p,
+           IndexTuple it    /* it has compressed entry */)
 {
+    Relation    r = istate->r;
+    GISTSTATE   *giststate = istate->giststate;
     OffsetNumber result;
     OffsetNumber maxoff;
     OffsetNumber i;
@@ -382,7 +388,7 @@ gistchoose(Relation r, Page p, IndexTuple it,    /* it has compressed entry */
 
     Assert(!GistPageIsLeaf(p));
 
-    gistDeCompressAtt(giststate, r,
+    gistDeCompressAtt(istate,
                       it, NULL, (OffsetNumber) 0,
                       identry, isnull);
 
@@ -568,9 +574,11 @@ gistdentryinit(GISTSTATE *giststate, int nkey, GISTENTRY *e,
 }
 
 IndexTuple
-gistFormTuple(GISTSTATE *giststate, Relation r,
+gistFormTuple(GISTInsertState *istate,
               Datum attdata[], bool isnull[], bool isleaf)
 {
+    Relation    r = istate->r;
+    GISTSTATE  *giststate = istate->giststate;
     Datum        compatt[INDEX_MAX_KEYS];
     int            i;
     IndexTuple    res;
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 36ed7244ba..6818e9d70e 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -239,9 +239,10 @@ typedef struct GistSplitVector
 
 typedef struct
 {
-    Relation    r;
+    Relation    r;                /* index relation  */
     Size        freespace;        /* free space to be left */
 
+    GISTSTATE        *giststate;
     GISTInsertStack *stack;
 } GISTInsertState;
 
@@ -386,10 +387,7 @@ extern bool gistinsert(Relation r, Datum *values, bool *isnull,
 extern MemoryContext createTempGistContext(void);
 extern GISTSTATE *initGISTstate(Relation index);
 extern void freeGISTstate(GISTSTATE *giststate);
-extern void gistdoinsert(Relation r,
-             IndexTuple itup,
-             Size freespace,
-             GISTSTATE *GISTstate);
+extern void gistdoinsert(GISTInsertState *istate, IndexTuple itup);
 
 /* A List of these is returned from gistplacetopage() in *splitinfo */
 typedef struct
@@ -398,7 +396,7 @@ typedef struct
     IndexTuple    downlink;        /* downlink for this half. */
 } GISTPageSplitInfo;
 
-extern bool gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
+extern bool gistplacetopage(GISTInsertState *istate,
                 Buffer buffer,
                 IndexTuple *itup, int ntup,
                 OffsetNumber oldoffnum, BlockNumber *newblkno,
@@ -406,8 +404,8 @@ extern bool gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
                 List **splitinfo,
                 bool markleftchild);
 
-extern SplitedPageLayout *gistSplit(Relation r, Page page, IndexTuple *itup,
-          int len, GISTSTATE *giststate);
+extern SplitedPageLayout *gistSplit(GISTInsertState *istate, Page page,
+                                    IndexTuple *itup, int len);
 
 extern XLogRecPtr gistXLogUpdate(Buffer buffer,
                OffsetNumber *todelete, int ntodelete,
@@ -451,18 +449,13 @@ extern IndexTuple *gistjoinvector(
                IndexTuple *additvec, int addlen);
 extern IndexTupleData *gistfillitupvec(IndexTuple *vec, int veclen, int *memlen);
 
-extern IndexTuple gistunion(Relation r, IndexTuple *itvec,
-          int len, GISTSTATE *giststate);
-extern IndexTuple gistgetadjusted(Relation r,
-                IndexTuple oldtup,
-                IndexTuple addtup,
-                GISTSTATE *giststate);
-extern IndexTuple gistFormTuple(GISTSTATE *giststate,
-              Relation r, Datum *attdata, bool *isnull, bool isleaf);
+extern IndexTuple gistunion(GISTInsertState *istate, IndexTuple *itvec, int len);
+extern IndexTuple gistgetadjusted(GISTInsertState *istate,
+                                  IndexTuple oldtup, IndexTuple addtup);
+extern IndexTuple gistFormTuple(GISTInsertState *istate,
+                                Datum *attdata, bool *isnull, bool isleaf);
 
-extern OffsetNumber gistchoose(Relation r, Page p,
-           IndexTuple it,
-           GISTSTATE *giststate);
+extern OffsetNumber gistchoose(GISTInsertState *istate, Page p, IndexTuple it);
 
 extern void GISTInitBuffer(Buffer b, uint32 f);
 extern void gistdentryinit(GISTSTATE *giststate, int nkey, GISTENTRY *e,
@@ -475,7 +468,7 @@ extern float gistpenalty(GISTSTATE *giststate, int attno,
 extern void gistMakeUnionItVec(GISTSTATE *giststate, IndexTuple *itvec, int len,
                    Datum *attr, bool *isnull);
 extern bool gistKeyIsEQ(GISTSTATE *giststate, int attno, Datum a, Datum b);
-extern void gistDeCompressAtt(GISTSTATE *giststate, Relation r, IndexTuple tuple, Page p,
+extern void gistDeCompressAtt(GISTInsertState *istate, IndexTuple tuple, Page p,
                   OffsetNumber o, GISTENTRY *attdata, bool *isnull);
 extern HeapTuple gistFetchTuple(GISTSTATE *giststate, Relation r,
                IndexTuple tuple);
@@ -495,10 +488,8 @@ extern IndexBulkDeleteResult *gistvacuumcleanup(IndexVacuumInfo *info,
                   IndexBulkDeleteResult *stats);
 
 /* gistsplit.c */
-extern void gistSplitByKey(Relation r, Page page, IndexTuple *itup,
-               int len, GISTSTATE *giststate,
-               GistSplitVector *v,
-               int attno);
+extern void gistSplitByKey(GISTInsertState *istate, Page page, IndexTuple *itup,
+               int len, GistSplitVector *v, int attno);
 
 /* gistbuild.c */
 extern IndexBuildResult *gistbuild(Relation heap, Relation index,
@@ -516,8 +507,8 @@ extern void gistPushItupToNodeBuffer(GISTBuildBuffers *gfbb,
 extern bool gistPopItupFromNodeBuffer(GISTBuildBuffers *gfbb,
                           GISTNodeBuffer *nodeBuffer, IndexTuple *item);
 extern void gistFreeBuildBuffers(GISTBuildBuffers *gfbb);
-extern void gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb,
-                                GISTSTATE *giststate, Relation r,
+extern void gistRelocateBuildBuffersOnSplit(GISTInsertState *istate,
+                                GISTBuildBuffers *gfbb,
                                 int level, Buffer buffer,
                                 List *splitinfo);
 extern void gistUnloadNodeBuffers(GISTBuildBuffers *gfbb);
-- 
2.16.3

From cfaf988364f75518389adc051b36b8b8dc5abc05 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 30 Aug 2018 14:35:40 +0900
Subject: [PATCH 2/2] Fix error message of gistSplit.

An error message of gistSplit looks wrong because it doesn't consider
fillfactor which the caller is considering. This patch corrects the
message by considering fillfactor.
---
 src/backend/access/gist/gist.c     | 7 ++++---
 src/backend/access/gist/gistutil.c | 5 ++---
 src/include/access/gist_private.h  | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 33b9532bff..4132fe2990 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -1364,7 +1364,8 @@ gistSplit(GISTInsertState *istate,
         ereport(ERROR,
                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
                  errmsg("index row size %zu exceeds maximum %zu for index \"%s\"",
-                        IndexTupleSize(itup[0]), GiSTPageSize,
+                        IndexTupleSize(itup[0]),
+                        GiSTPageSize - istate->freespace,
                         RelationGetRelationName(istate->r))));
 
     memset(v.spl_lisnull, true, sizeof(bool) * giststate->tupdesc->natts);
@@ -1382,7 +1383,7 @@ gistSplit(GISTInsertState *istate,
         rvectup[i] = itup[v.splitVector.spl_right[i] - 1];
 
     /* finalize splitting (may need another split) */
-    if (!gistfitpage(rvectup, v.splitVector.spl_nright))
+    if (!gistfitpage(rvectup, v.splitVector.spl_nright, istate->freespace))
     {
         res = gistSplit(istate, page, rvectup, v.splitVector.spl_nright);
     }
@@ -1394,7 +1395,7 @@ gistSplit(GISTInsertState *istate,
         res->itup = gistFormTuple(istate, v.spl_rattr, v.spl_risnull, false);
     }
 
-    if (!gistfitpage(lvectup, v.splitVector.spl_nleft))
+    if (!gistfitpage(lvectup, v.splitVector.spl_nleft, istate->freespace))
     {
         SplitedPageLayout *resptr,
                    *subres;
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 20802f1adc..286068666c 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -74,7 +74,7 @@ gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete, Size f
 }
 
 bool
-gistfitpage(IndexTuple *itvec, int len)
+gistfitpage(IndexTuple *itvec, int len, int freespace)
 {
     int            i;
     Size        size = 0;
@@ -82,8 +82,7 @@ gistfitpage(IndexTuple *itvec, int len)
     for (i = 0; i < len; i++)
         size += IndexTupleSize(itvec[i]) + sizeof(ItemIdData);
 
-    /* TODO: Consider fillfactor */
-    return (size <= GiSTPageSize);
+    return (size <= GiSTPageSize - freespace);
 }
 
 /*
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 6818e9d70e..cf15173e6f 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -437,7 +437,7 @@ extern bytea *gistoptions(Datum reloptions, bool validate);
 extern bool gistproperty(Oid index_oid, int attno,
              IndexAMProperty prop, const char *propname,
              bool *res, bool *isnull);
-extern bool gistfitpage(IndexTuple *itvec, int len);
+extern bool gistfitpage(IndexTuple *itvec, int len, int freespace);
 extern bool gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete, Size freespace);
 extern void gistcheckpage(Relation rel, Buffer buf);
 extern Buffer gistNewBuffer(Relation r);
-- 
2.16.3


Re: A strange GiST error message or fillfactor of GiST build

От
Andrey Borodin
Дата:
Hello!

> 30 авг. 2018 г., в 2:42, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> написал(а):
>
> At Wed, 29 Aug 2018 10:42:59 -0300, Andrey Borodin <x4mmm@yandex-team.ru> wrote in
<6FBE12B2-4F59-4DB9-BDE9-62C8801189A8@yandex-team.ru>
>>
>> We are passing freespace everywhere. Also, we pass GistInsertState, and GistState.
>> Maybe let's put GistState into GistInsertState, GistState already has free space, and pass just GistInsertState
everywhere?
>
> Yeah, I thought something like that first. GISTSTATE doesn't have
> freespace size but we could refactor so that all insert-related
> routines use GISTInsertState and make GISTBuildState have
> it. (patch 1) But this prevents future back-patching so I don't
> think this acceptable.

The patch looks good to me. Making code better for future development seem to me more important than backpatching this:
errorper se is cryptic but not threatening, it will be prevented by cube construction routines limitations. 
But that is just my IMHO.

Best regards, Andrey Borodin.

Re: A strange GiST error message or fillfactor of GiST build

От
Robert Haas
Дата:
On Wed, Aug 29, 2018 at 4:32 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> After the attached patch applied, the above messages becomes as
> follows. (And index can be built being a bit sparse by fill
> factor.)
>
>> ERROR:  index row size 8016 exceeds maximum 7333 for index "y_cube_idx"
>
> I'm not sure why 277807bd9e didn't do that completely so I may be
> missing something. Is there any thoughts?

It seems strange to me that we consider respecting the fillfactor to
be more important than letting the operation succeed.  I would have
thought that the fillfactor would not apply when placing a tuple into
a completely empty page.  The point of the setting is, of course, to
leave some free space available on the page for future tuples, but if
the tuples are big enough that only one fits in a page anyway, that's
pointless.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: A strange GiST error message or fillfactor of GiST build

От
Alexander Korotkov
Дата:
On Sat, Sep 1, 2018 at 6:03 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Aug 29, 2018 at 4:32 AM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > After the attached patch applied, the above messages becomes as
> > follows. (And index can be built being a bit sparse by fill
> > factor.)
> >
> >> ERROR:  index row size 8016 exceeds maximum 7333 for index "y_cube_idx"
> >
> > I'm not sure why 277807bd9e didn't do that completely so I may be
> > missing something. Is there any thoughts?
>
> It seems strange to me that we consider respecting the fillfactor to
> be more important than letting the operation succeed.  I would have
> thought that the fillfactor would not apply when placing a tuple into
> a completely empty page.  The point of the setting is, of course, to
> leave some free space available on the page for future tuples, but if
> the tuples are big enough that only one fits in a page anyway, that's
> pointless.

IIRC, I've already wrote that I think we don't need GiST fillfactor
parameter at all.  As you pointed, the purpose of fillfactor parameter
is to leave some free space in the pages.  That, in turn, allow us to
evade the flood of page splits, which may happen when you start
insertions into freshly build and perfectly packed index.  But thats
makes sense only for index building algorithms, which can pack index
pages as tight as possible.  Our B-tree build algorithm is one of such
alogirhtms: at first it sorts tuples and then packs them into pages as
tight as required.  But GiST is another story: GiST index build in the
pretty same as insertion tuples one by one.  Yes, we have some bulk
insert optimization for GiST, but it optimizes only IO and internally
still uses picksplit.  So, GiST indexes are never perfectly packed
even with fillfactor = 100.  Why should we bother setting lower
fillfactor?

Thus, I would vote for removing GiST fillfactor altogether.  Assuming
we can't do this for compatibility reasons, I would vote for setting
default GiST fillfactor to 100, and don't introduce new places where
we take it into account.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Re: A strange GiST error message or fillfactor of GiST build

От
Tom Lane
Дата:
Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> Thus, I would vote for removing GiST fillfactor altogether.  Assuming
> we can't do this for compatibility reasons, I would vote for setting
> default GiST fillfactor to 100, and don't introduce new places where
> we take it into account.

We probably can't remove the fillfactor storage parameter, both for
backwards compatibility and because I think it's implemented independently
of index type.  But there's no backwards-compatibility argument against
simply ignoring it, if we conclude it's a bad idea.

            regards, tom lane


Re: A strange GiST error message or fillfactor of GiST build

От
Alexander Korotkov
Дата:
On Sat, Sep 1, 2018 at 9:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> > Thus, I would vote for removing GiST fillfactor altogether.  Assuming
> > we can't do this for compatibility reasons, I would vote for setting
> > default GiST fillfactor to 100, and don't introduce new places where
> > we take it into account.
>
> We probably can't remove the fillfactor storage parameter, both for
> backwards compatibility and because I think it's implemented independently
> of index type.  But there's no backwards-compatibility argument against
> simply ignoring it, if we conclude it's a bad idea.

That's a good idea.  Especially if we take into account that
fillfactor is not a forever bad idea for GIST, it just doesn't look
reasonable for current building algorithm.  So, we can decide to
ignore it, but if we would switch to different GiST building
algorithm, which can pack pages tightly, we can take fillfactor into
account again.

I think I need to prove my position about GiST fillfactor with some
experiments first, and then propose a patch.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Re: A strange GiST error message or fillfactor of GiST build

От
Andrey Borodin
Дата:

> 3 сент. 2018 г., в 20:16, Alexander Korotkov <a.korotkov@postgrespro.ru> написал(а):
>
> That's a good idea.  Especially if we take into account that
> fillfactor is not a forever bad idea for GIST, it just doesn't look
> reasonable for current building algorithm.  So, we can decide to
> ignore it, but if we would switch to different GiST building
> algorithm, which can pack pages tightly, we can take fillfactor into
> account again.
BTW, I think that build algorithm may be provided by opclass.
>
> I think I need to prove my position about GiST fillfactor with some
> experiments first, and then propose a patch.
FWIW fillfactor can be a cheap emulator for intrapage indexing, when you have like a lot of RAM. Though I didn't tested
that.
Also I believe proper intrapage indexing is better :)

Best regards, Andrey Borodin.

Re: A strange GiST error message or fillfactor of GiST build

От
Alexander Korotkov
Дата:
On Mon, Sep 3, 2018 at 9:09 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > 3 сент. 2018 г., в 20:16, Alexander Korotkov <a.korotkov@postgrespro.ru> написал(а):
> > That's a good idea.  Especially if we take into account that
> > fillfactor is not a forever bad idea for GIST, it just doesn't look
> > reasonable for current building algorithm.  So, we can decide to
> > ignore it, but if we would switch to different GiST building
> > algorithm, which can pack pages tightly, we can take fillfactor into
> > account again.
> BTW, I think that build algorithm may be provided by opclass.

I don't think that providing the whole building algorithm by opclass
is a good idea.  But we could rather make opclass provide some
essential routines for build algorithm.  For instance, we may
implement sorting-based build algorithm for GiST (like one we have for
B-tree).  It wouldn't work for all the opclass'es, but definitely
should work for some of them.  Geometrical opclass'es may sort values
by some kind of space-filling curve.  In this case, opclass can define
a comparison function.

> > I think I need to prove my position about GiST fillfactor with some
> > experiments first, and then propose a patch.
> FWIW fillfactor can be a cheap emulator for intrapage indexing, when you have like a lot of RAM. Though I didn't
testedthat. 
> Also I believe proper intrapage indexing is better :)

It might be somewhat useful side effect for developers.  But it's
definitely doesn't look like a feature we should encourage users to
use.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Re: A strange GiST error message or fillfactor of GiST build

От
Kyotaro HORIGUCHI
Дата:
At Mon, 3 Sep 2018 23:05:23 +0300, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote in
<CAPpHfdsruk4eEkVTUFTHV7tg9RXQHqvOvBY18p-5pBNqp+PU6w@mail.gmail.com>
> On Mon, Sep 3, 2018 at 9:09 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > > 3 сент. 2018 г., в 20:16, Alexander Korotkov <a.korotkov@postgrespro.ru> написал(а):
> > > That's a good idea.  Especially if we take into account that
> > > fillfactor is not a forever bad idea for GIST, it just doesn't look
> > > reasonable for current building algorithm.  So, we can decide to
> > > ignore it, but if we would switch to different GiST building
> > > algorithm, which can pack pages tightly, we can take fillfactor into
> > > account again.
> > BTW, I think that build algorithm may be provided by opclass.
>
> I don't think that providing the whole building algorithm by opclass
> is a good idea.  But we could rather make opclass provide some
> essential routines for build algorithm.  For instance, we may
> implement sorting-based build algorithm for GiST (like one we have for
> B-tree).  It wouldn't work for all the opclass'es, but definitely
> should work for some of them.  Geometrical opclass'es may sort values
> by some kind of space-filling curve.  In this case, opclass can define
> a comparison function.
>
> > > I think I need to prove my position about GiST fillfactor with some
> > > experiments first, and then propose a patch.
> > FWIW fillfactor can be a cheap emulator for intrapage indexing, when you have like a lot of RAM. Though I didn't
testedthat. 
> > Also I believe proper intrapage indexing is better :)
>
> It might be somewhat useful side effect for developers.  But it's
> definitely doesn't look like a feature we should encourage users to
> use.

I agree that fillfactor should be ignored in certain cases
(inserting the first tuple into empty pages or something like
that). Even though GiST doesn't need fillfactor at all, it is
defined independently from AM instances and it gives some
(undocumented) convenient on testing even on GiST.

So, does the following makes sense?

- Preserve fillfactor on GiST but set its default to 100%.

- Ignore fillfactor iff the first tuple for the new page fail
  with it but succeed without it.

- Modify GiST internal routines to bring around GISTInsertState
  so that they can see fillfactor or any other parameters without
  further modification.

  https://www.postgresql.org/message-id/20180830.144209.208080135.horiguchi.kyotaro@lab.ntt.co.jp

# A storm (literally) is coming behind...

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center





Re: A strange GiST error message or fillfactor of GiST build

От
Alexander Korotkov
Дата:
On Tue, Sep 4, 2018 at 12:16 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> I agree that fillfactor should be ignored in certain cases
> (inserting the first tuple into empty pages or something like
> that). Even though GiST doesn't need fillfactor at all, it is
> defined independently from AM instances

What exactly do you mean?  Yes, fillfactor is defined in StdRdOptions
struct, which is shared across many access methods.  But some of them
uses fillfactor, while others don't.  For instance, GIN and BRIN don't
support fillfactor at all.

> and it gives some
> (undocumented) convenient on testing even on GiST.

Do you keep in the mind some particular use cases?  If so, could you
please share them.  It would let me and others understand what
behavior we need to preserve and why.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Re: A strange GiST error message or fillfactor of GiST build

От
Kyotaro HORIGUCHI
Дата:
At Tue, 4 Sep 2018 13:05:55 +0300, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote in
<CAPpHfdsJwptVsRnBtTdtj+G+gTTfSrqdH1uwLNkvQ-72A9scgw@mail.gmail.com>
> On Tue, Sep 4, 2018 at 12:16 PM Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > I agree that fillfactor should be ignored in certain cases
> > (inserting the first tuple into empty pages or something like
> > that). Even though GiST doesn't need fillfactor at all, it is
> > defined independently from AM instances
> 
> What exactly do you mean?  Yes, fillfactor is defined in StdRdOptions
> struct, which is shared across many access methods.  But some of them
> uses fillfactor, while others don't.  For instance, GIN and BRIN don't
> support fillfactor at all.

Yes. It was with the intent of keeping Andrey's use case. So I
proposed that just disabling it rather than removing the code at
all.

> > and it gives some
> > (undocumented) convenient on testing even on GiST.
> 
> Do you keep in the mind some particular use cases?  If so, could you
> please share them.  It would let me and others understand what
> behavior we need to preserve and why.

I misunderstood the consensus here, I myself don't have a proper
one. The consensus here is:

fillfactor is useless for GiST.
We don't preserve it for Andrey's use case.
No one is objecting to ignoring it here.

Is it right?

Well, I propose the first attached instaed. It just removes
fillfactor handling from GiST.

Apart from the fillfactor removal, we have an internal error for
rootsplit failure caused by too-long tuples, but this should be a
user error message like gistSplit. (index row size %zu exeeds..)

=# create table y as  select cube(array(SELECT random() as a FROM generate_series(1,600))) from
generate_series(1,1e3,1);
 
=# create index on y using gist(cube);
ERROR:  failed to add item to index page in "y_cube_idx"

The attached second patch changes the  message in the case.

ERROR:  size of 2 index rows (9640 bytes) exceeds maximum 8152 of the root page for the index "y_cube_idx"

This is one of my first motivation here but now this might be
another issue.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From e5abf14d5d7de4256a7d40a0e5783655a99c2515 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 5 Sep 2018 09:49:58 +0900
Subject: [PATCH 1/2] Ignore fillfactor in GiST

fillfactor for GiST doesn't work as expected since GiST doesn't
perform sorted bulk insertion to pack pages as tight as
possible. Therefore we remove the fillfactor capability from it. It is
still allowed to be set for backward compatibility but just ignored.
---
 doc/src/sgml/ref/create_index.sgml     |  8 +++++++-
 src/backend/access/common/reloptions.c |  4 ++--
 src/backend/access/gist/gist.c         | 13 ++++++-------
 src/backend/access/gist/gistbuild.c    | 18 +++---------------
 src/backend/access/gist/gistutil.c     |  5 ++---
 src/include/access/gist_private.h      | 12 +++---------
 6 files changed, 23 insertions(+), 37 deletions(-)

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 3c1223b324..e2a77e0d4d 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -390,7 +390,7 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
    </variablelist>
 
    <para>
-     The B-tree, hash, GiST and SP-GiST index methods all accept this parameter:
+     The B-tree, hash and SP-GiST index methods all accept this parameter:
    </para>
 
    <variablelist>
@@ -412,6 +412,12 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
       other index methods use fillfactor in different but roughly analogous
       ways; the default fillfactor varies between methods.
      </para>
+     <note>
+       <para>
+         GiST also accepts this parameter just for historical reasons but
+         ignores it.
+       </para>
+     </note>
     </listitem>
    </varlistentry>
    </variablelist>
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index db84da0678..9c9589a78e 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -186,12 +186,12 @@ static relopt_int intRelOpts[] =
     {
         {
             "fillfactor",
-            "Packs gist index pages only to this percentage",
+            "This is ignored but exists for historical reasons",
             RELOPT_KIND_GIST,
             ShareUpdateExclusiveLock    /* since it applies only to later
                                          * inserts */
         },
-        GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
+        0, 10, 100
     },
     {
         {
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 8a42effdf7..5915ab2cf2 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -172,7 +172,7 @@ gistinsert(Relation r, Datum *values, bool *isnull,
                          values, isnull, true /* size is currently bogus */ );
     itup->t_tid = *ht_ctid;
 
-    gistdoinsert(r, itup, 0, giststate);
+    gistdoinsert(r, itup, giststate);
 
     /* cleanup */
     MemoryContextSwitchTo(oldCxt);
@@ -212,7 +212,7 @@ gistinsert(Relation r, Datum *values, bool *isnull,
  * Returns 'true' if the page was split, 'false' otherwise.
  */
 bool
-gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
+gistplacetopage(Relation rel, GISTSTATE *giststate,
                 Buffer buffer,
                 IndexTuple *itup, int ntup, OffsetNumber oldoffnum,
                 BlockNumber *newblkno,
@@ -251,7 +251,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
      * one-element todelete array; in the split case, it's handled implicitly
      * because the tuple vector passed to gistSplit won't include this tuple.
      */
-    is_split = gistnospace(page, itup, ntup, oldoffnum, freespace);
+    is_split = gistnospace(page, itup, ntup, oldoffnum);
 
     /*
      * If leaf page is full, try at first to delete dead tuples. And then
@@ -260,7 +260,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
     if (is_split && GistPageIsLeaf(page) && GistPageHasGarbage(page))
     {
         gistvacuumpage(rel, page, buffer);
-        is_split = gistnospace(page, itup, ntup, oldoffnum, freespace);
+        is_split = gistnospace(page, itup, ntup, oldoffnum);
     }
 
     if (is_split)
@@ -604,7 +604,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
  * so it does not bother releasing palloc'd allocations.
  */
 void
-gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
+gistdoinsert(Relation r, IndexTuple itup, GISTSTATE *giststate)
 {
     ItemId        iid;
     IndexTuple    idxtuple;
@@ -614,7 +614,6 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace, GISTSTATE *giststate)
     bool        xlocked = false;
 
     memset(&state, 0, sizeof(GISTInsertState));
-    state.freespace = freespace;
     state.r = r;
 
     /* Start from the root */
@@ -1226,7 +1225,7 @@ gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
     CheckForSerializableConflictIn(state->r, NULL, stack->buffer);
 
     /* Insert the tuple(s) to the page, splitting the page if necessary */
-    is_split = gistplacetopage(state->r, state->freespace, giststate,
+    is_split = gistplacetopage(state->r, giststate,
                                stack->buffer,
                                tuples, ntup,
                                oldoffnum, NULL,
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 434f15f014..cd588cfceb 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -61,8 +61,6 @@ typedef struct
     int64        indtuples;        /* number of tuples indexed */
     int64        indtuplesSize;    /* total size of all indexed tuples */
 
-    Size        freespace;        /* amount of free space to leave on pages */
-
     /*
      * Extra data structures used during a buffering build. 'gfbb' contains
      * information related to managing the build buffers. 'parentMap' is a
@@ -119,7 +117,6 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
     Buffer        buffer;
     Page        page;
     MemoryContext oldcxt = CurrentMemoryContext;
-    int            fillfactor;
 
     buildstate.indexrel = index;
     if (index->rd_options)
@@ -134,8 +131,6 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
             buildstate.bufferingMode = GIST_BUFFERING_DISABLED;
         else
             buildstate.bufferingMode = GIST_BUFFERING_AUTO;
-
-        fillfactor = options->fillfactor;
     }
     else
     {
@@ -144,10 +139,7 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
          * to fit in cache.
          */
         buildstate.bufferingMode = GIST_BUFFERING_AUTO;
-        fillfactor = GIST_DEFAULT_FILLFACTOR;
     }
-    /* Calculate target amount of free space to leave on pages */
-    buildstate.freespace = BLCKSZ * (100 - fillfactor) / 100;
 
     /*
      * We expect to be called exactly once for any index relation. If that's
@@ -275,8 +267,7 @@ gistInitBuffering(GISTBuildState *buildstate)
 
     /* Calc space of index page which is available for index tuples */
     pageFreeSpace = BLCKSZ - SizeOfPageHeaderData - sizeof(GISTPageOpaqueData)
-        - sizeof(ItemIdData)
-        - buildstate->freespace;
+        - sizeof(ItemIdData);
 
     /*
      * Calculate average size of already inserted index tuples using gathered
@@ -431,8 +422,7 @@ calculatePagesPerBuffer(GISTBuildState *buildstate, int levelStep)
 
     /* Calc space of index page which is available for index tuples */
     pageFreeSpace = BLCKSZ - SizeOfPageHeaderData - sizeof(GISTPageOpaqueData)
-        - sizeof(ItemIdData)
-        - buildstate->freespace;
+        - sizeof(ItemIdData);
 
     /*
      * Calculate average size of already inserted index tuples using gathered
@@ -483,8 +473,7 @@ gistBuildCallback(Relation index,
          * There's no buffers (yet). Since we already have the index relation
          * locked, we call gistdoinsert directly.
          */
-        gistdoinsert(index, itup, buildstate->freespace,
-                     buildstate->giststate);
+        gistdoinsert(index, itup, buildstate->giststate);
     }
 
     /* Update tuple count and total size. */
@@ -684,7 +673,6 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
     BlockNumber placed_to_blk = InvalidBlockNumber;
 
     is_split = gistplacetopage(buildstate->indexrel,
-                               buildstate->freespace,
                                buildstate->giststate,
                                buffer,
                                itup, ntup, oldoffnum, &placed_to_blk,
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index dddfe0ae2c..89aad95b20 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -54,9 +54,9 @@ gistfillbuffer(Page page, IndexTuple *itup, int len, OffsetNumber off)
  * Check space for itup vector on page
  */
 bool
-gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete, Size freespace)
+gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete)
 {
-    unsigned int size = freespace,
+    unsigned int size = 0,
                 deleted = 0;
     int            i;
 
@@ -82,7 +82,6 @@ gistfitpage(IndexTuple *itvec, int len)
     for (i = 0; i < len; i++)
         size += IndexTupleSize(itvec[i]) + sizeof(ItemIdData);
 
-    /* TODO: Consider fillfactor */
     return (size <= GiSTPageSize);
 }
 
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 36ed7244ba..dd8845b6be 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -240,8 +240,6 @@ typedef struct GistSplitVector
 typedef struct
 {
     Relation    r;
-    Size        freespace;        /* free space to be left */
-
     GISTInsertStack *stack;
 } GISTInsertState;
 
@@ -373,7 +371,7 @@ typedef struct GISTBuildBuffers
 typedef struct GiSTOptions
 {
     int32        vl_len_;        /* varlena header (do not touch directly!) */
-    int            fillfactor;        /* page fill factor in percent (0..100) */
+    int            fillfactor;        /* page fill factor: JUST IGNORED  */
     int            bufferingModeOffset;    /* use buffering build? */
 } GiSTOptions;
 
@@ -388,7 +386,6 @@ extern GISTSTATE *initGISTstate(Relation index);
 extern void freeGISTstate(GISTSTATE *giststate);
 extern void gistdoinsert(Relation r,
              IndexTuple itup,
-             Size freespace,
              GISTSTATE *GISTstate);
 
 /* A List of these is returned from gistplacetopage() in *splitinfo */
@@ -398,7 +395,7 @@ typedef struct
     IndexTuple    downlink;        /* downlink for this half. */
 } GISTPageSplitInfo;
 
-extern bool gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
+extern bool gistplacetopage(Relation rel, GISTSTATE *giststate,
                 Buffer buffer,
                 IndexTuple *itup, int ntup,
                 OffsetNumber oldoffnum, BlockNumber *newblkno,
@@ -432,15 +429,12 @@ extern bool gistvalidate(Oid opclassoid);
 #define GiSTPageSize   \
     ( BLCKSZ - SizeOfPageHeaderData - MAXALIGN(sizeof(GISTPageOpaqueData)) )
 
-#define GIST_MIN_FILLFACTOR            10
-#define GIST_DEFAULT_FILLFACTOR        90
-
 extern bytea *gistoptions(Datum reloptions, bool validate);
 extern bool gistproperty(Oid index_oid, int attno,
              IndexAMProperty prop, const char *propname,
              bool *res, bool *isnull);
 extern bool gistfitpage(IndexTuple *itvec, int len);
-extern bool gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete, Size freespace);
+extern bool gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete);
 extern void gistcheckpage(Relation rel, Buffer buf);
 extern Buffer gistNewBuffer(Relation r);
 extern void gistfillbuffer(Page page, IndexTuple *itup, int len,
-- 
2.16.3

From e1418b6655386eb34574c4f930d8dc66f16e69a1 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 5 Sep 2018 12:15:13 +0900
Subject: [PATCH 2/2] More friendly error message for rootsplit failure of GiST

When tuple insertion failed during page split of GiST index, we have
an internal error that complains that just "failed to add item to
index page". However, things are a bit different in the rootsplit
case. It is rather a user side error that the user provided too long
data for the index so emit a more friendly error message in the case
like gistSplit does.
---
 src/backend/access/gist/gist.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 5915ab2cf2..4590e88a77 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -412,7 +412,34 @@ gistplacetopage(Relation rel, GISTSTATE *giststate,
                 IndexTuple    thistup = (IndexTuple) data;
 
                 if (PageAddItem(ptr->page, (Item) data, IndexTupleSize(thistup), i + FirstOffsetNumber, false, false)
==InvalidOffsetNumber)
 
+                {
+                    /*
+                     * Emit user error message for root split failure since
+                     * this is rather a user error than internal one. We could
+                     * this earlier before preparing new buffers, but we don't
+                     * bother prechecking for such a corner case.
+                     */
+                    if (is_rootsplit && ptr == dist)
+                    {
+                        size_t    total_tupsize;
+
+                        /* count size of all tuples to be inserted */
+                        for (; i < ptr->block.num ; i++)
+                            data += IndexTupleSize((IndexTuple) data);
+
+                        /*  total_tupsize is including item id */
+                        total_tupsize =
+                            (char *)data - (char *) (ptr->list)
+                            + sizeof(ItemIdData) * ptr->block.num;
+                        ereport(ERROR,
+                                (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+                                 (errmsg("size of %d index rows (%zu bytes) exceeds maximum %zu of the root page for
theindex \"%s\"",
 
+                                         ptr->block.num, total_tupsize,
+                                         GiSTPageSize,
+                                         RelationGetRelationName(rel)))));
+                    }
                     elog(ERROR, "failed to add item to index page in \"%s\"", RelationGetRelationName(rel));
+                }
 
                 /*
                  * If this is the first inserted/updated tuple, let the caller
-- 
2.16.3


Re: A strange GiST error message or fillfactor of GiST build

От
Andrey Borodin
Дата:

> 5 сент. 2018 г., в 13:29, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> написал(а):
>
> We don't preserve it for Andrey's use case.
Just my 2 cents: that was a hacky use case for development reasons. I think that removing fillfactor is good idea and
yourlatest patch looks good from my POV. 

Best regards, Andrey Borodin.

Re: A strange GiST error message or fillfactor of GiST build

От
Kyotaro HORIGUCHI
Дата:
Hello.

 > Just my 2 cents: that was a hacky use case for development reasons.

I know. So I intended to preserve the parameter with default of 100% if  
no one strongly objects to preserve. Then I abandoned that since I had:p

 > I think that removing fillfactor is good idea and your latest patch
 > looks good from my POV.

Thanks.

reagrds.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: A strange GiST error message or fillfactor of GiST build

От
Bruce Momjian
Дата:
On Thu, Sep  6, 2018 at 04:16:45PM +0900, Kyotaro HORIGUCHI wrote:
> Hello.
> 
> > Just my 2 cents: that was a hacky use case for development reasons.
> 
> I know. So I intended to preserve the parameter with default of 100% if no
> one strongly objects to preserve. Then I abandoned that since I had:p

So, are we going to output a notice if a non-100% fill factor is
specified?

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +


Re: A strange GiST error message or fillfactor of GiST build

От
Tom Lane
Дата:
Bruce Momjian <bruce@momjian.us> writes:
> So, are we going to output a notice if a non-100% fill factor is
> specified?

I would not think that's helpful.

            regards, tom lane


Re: A strange GiST error message or fillfactor of GiST build

От
Kyotaro HORIGUCHI
Дата:
Hello.

At Fri, 07 Sep 2018 16:12:29 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <19695.1536351149@sss.pgh.pa.us>
> Bruce Momjian <bruce@momjian.us> writes:
> > So, are we going to output a notice if a non-100% fill factor is
> > specified?
> 
> I would not think that's helpful.

I agree. My understanding is that:

It has'nt been worked as expected anyway thus we ignore it
instead of removing not to break existing setup, and with silence
not to surprise someone using it believing it works.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center