Re: A strange GiST error message or fillfactor of GiST build
Hello. At Fri, 07 Sep 2018 16:12:29 -0400, Tom Lane wrote in <19695.1536351...@sss.pgh.pa.us> > Bruce Momjian 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
Re: A strange GiST error message or fillfactor of GiST build
Bruce Momjian 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
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 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
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
> 5 сент. 2018 г., в 13:29, Kyotaro HORIGUCHI > написал(а): > > 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 your latest patch looks good from my POV. Best regards, Andrey Borodin.
Re: A strange GiST error message or fillfactor of GiST build
At Tue, 4 Sep 2018 13:05:55 +0300, Alexander Korotkov
wrote in
> On Tue, Sep 4, 2018 at 12:16 PM Kyotaro HORIGUCHI
> 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
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 ]
- 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:
@@ -412,6 +412,12 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ]
+
+
+ GiST also accepts this parameter just for historical reasons but
+ ignores it.
+
+
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
Re: A strange GiST error message or fillfactor of GiST build
On Tue, Sep 4, 2018 at 12:16 PM Kyotaro HORIGUCHI 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
At Mon, 3 Sep 2018 23:05:23 +0300, Alexander Korotkov wrote in > On Mon, Sep 3, 2018 at 9:09 PM Andrey Borodin wrote: > > > 3 сент. 2018 г., в 20:16, Alexander Korotkov > > > написал(а): > > > 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 tested that. > > 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.kyot...@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
On Mon, Sep 3, 2018 at 9:09 PM Andrey Borodin wrote: > > 3 сент. 2018 г., в 20:16, Alexander Korotkov > > написал(а): > > 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 tested that. > 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
> 3 сент. 2018 г., в 20:16, Alexander Korotkov > написал(а): > > 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
On Sat, Sep 1, 2018 at 9:45 PM Tom Lane wrote: > Alexander Korotkov 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
Alexander Korotkov 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
On Sat, Sep 1, 2018 at 6:03 PM Robert Haas wrote: > On Wed, Aug 29, 2018 at 4:32 AM, Kyotaro HORIGUCHI > 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
On Wed, Aug 29, 2018 at 4:32 AM, Kyotaro HORIGUCHI 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
Hello! > 30 авг. 2018 г., в 2:42, Kyotaro HORIGUCHI > написал(а): > > At Wed, 29 Aug 2018 10:42:59 -0300, Andrey Borodin > wrote in <6fbe12b2-4f59-4db9-bde9-62c880118...@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: error per 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
Hello.
At Wed, 29 Aug 2018 10:42:59 -0300, Andrey Borodin wrote
in <6fbe12b2-4f59-4db9-bde9-62c880118...@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
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(,
values, isnull, true /* size is currently bogus */ );
itup->t_tid = *ht_ctid;
- gistdoinsert(r, itup, 0, giststate);
+ gistdoinsert(, 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
Re: A strange GiST error message or fillfactor of GiST build
Hi! > 29 авг. 2018 г., в 5:32, 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-06355b537...@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.
A strange GiST error message or fillfactor of GiST build
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-06355b537...@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..7773a2 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, , 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,
