From b41c6dca0fa646aff53348c05b0ea52e46e39dba Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Mon, 10 Aug 2026 12:01:07 +0500 Subject: [PATCH v1] Honor fillfactor in sorted GiST builds Sorted GiST builds have ignored fillfactor since they started using the opclass picksplit function. This is surprising because GiST accepts and otherwise honors the option, and sorted builds are selected by default when the opclass provides sortsupport. Pass the target free space through gistSplit() while partitioning buffered tuples. Treat fillfactor as a soft limit: allow a physically valid page to exceed it when the opclass split would create a singleton page. Physical page capacity remains a hard limit, and ordinary GiST page splits retain their existing behavior. Test both that a lower fillfactor produces a larger sorted index and that large tuples can exceed the soft limit without forcing singleton pages. Discussion: https://postgr.es/m/fbbfe5dc-3dfa-d54a-3a94-e2bee37b85d8@gmail.com --- contrib/pageinspect/expected/gist.out | 5 +-- contrib/pageinspect/sql/gist.sql | 5 +-- src/backend/access/gist/gist.c | 49 +++++++++++++++++++++++---- src/backend/access/gist/gistbuild.c | 15 +++++--- src/backend/access/gist/gistutil.c | 5 ++- src/include/access/gist_private.h | 5 +-- src/test/regress/expected/gist.out | 21 ++++++++++++ src/test/regress/sql/gist.sql | 18 ++++++++++ 8 files changed, 103 insertions(+), 20 deletions(-) diff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.out index 8502f9efb41..74a21bf32aa 100644 --- a/contrib/pageinspect/expected/gist.out +++ b/contrib/pageinspect/expected/gist.out @@ -2,7 +2,8 @@ -- Use an unlogged index, so that the LSN is predictable. CREATE UNLOGGED TABLE test_gist AS SELECT point(i,i) p, i::text t FROM generate_series(1,1000) i; -CREATE INDEX test_gist_idx ON test_gist USING gist (p); +CREATE INDEX test_gist_idx ON test_gist USING gist (p) + WITH (fillfactor = 100); -- Page 0 is the root, the rest are leaf pages SELECT * FROM gist_page_opaque_info(get_raw_page('test_gist_idx', 0)); lsn | nsn | rightlink | flags @@ -104,7 +105,7 @@ SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex')); -- the included attributes. ALTER TABLE test_gist ADD COLUMN i int DEFAULT NULL; CREATE INDEX test_gist_idx_inc ON test_gist - USING gist (p) INCLUDE (t, i); + USING gist (p) INCLUDE (t, i) WITH (fillfactor = 100); -- Mask the value of the key attribute to avoid alignment issues. SELECT regexp_replace(keys, '\(p\)=\("(.*?)"\)', '(p)=("")') AS keys_nonleaf_1 FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc') diff --git a/contrib/pageinspect/sql/gist.sql b/contrib/pageinspect/sql/gist.sql index 85bc44b8000..d9a2f4d4bb9 100644 --- a/contrib/pageinspect/sql/gist.sql +++ b/contrib/pageinspect/sql/gist.sql @@ -2,7 +2,8 @@ -- Use an unlogged index, so that the LSN is predictable. CREATE UNLOGGED TABLE test_gist AS SELECT point(i,i) p, i::text t FROM generate_series(1,1000) i; -CREATE INDEX test_gist_idx ON test_gist USING gist (p); +CREATE INDEX test_gist_idx ON test_gist USING gist (p) + WITH (fillfactor = 100); -- Page 0 is the root, the rest are leaf pages SELECT * FROM gist_page_opaque_info(get_raw_page('test_gist_idx', 0)); @@ -47,7 +48,7 @@ SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex')); -- the included attributes. ALTER TABLE test_gist ADD COLUMN i int DEFAULT NULL; CREATE INDEX test_gist_idx_inc ON test_gist - USING gist (p) INCLUDE (t, i); + USING gist (p) INCLUDE (t, i) WITH (fillfactor = 100); -- Mask the value of the key attribute to avoid alignment issues. SELECT regexp_replace(keys, '\(p\)=\("(.*?)"\)', '(p)=("")') AS keys_nonleaf_1 FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc') diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c index 8565e225be7..ca3a12f1174 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -313,7 +313,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, giststate, 0); /* * Check that split didn't produce too many pages. @@ -1441,29 +1441,56 @@ gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack, stack->retry_from_parent = true; } +/* Form a one-page layout without splitting the tuple vector. */ +static SplitPageLayout * +gistnosplit(Relation r, IndexTuple *itup, int len, GISTSTATE *giststate) +{ + SplitPageLayout *res = palloc0_object(SplitPageLayout); + + res->block.num = len; + res->list = gistfillitupvec(itup, len, &res->lenlist); + res->itup = gistunion(r, itup, len, giststate); + + return res; +} + /* * gistSplit -- split a page in the tree and fill struct * used for XLOG and real writes buffers. Function is recursive, ie * it will split page until keys will fit in every page. + * + * If freespace is nonzero, it is a soft target for unused space on each + * resulting page. In that case this can return a single-page layout when + * satisfying the target would create a singleton page. */ SplitPageLayout * gistSplit(Relation r, Page page, IndexTuple *itup, /* contains compressed entry */ int len, - GISTSTATE *giststate) + GISTSTATE *giststate, + Size freespace) { IndexTuple *lvectup, *rvectup; GistSplitVector v; int i; SplitPageLayout *res = NULL; + bool physical_fit; /* this should never recurse very deeply, but better safe than sorry */ check_stack_depth(); - /* there's no point in splitting an empty page */ + /* There's no point in splitting an empty page. */ Assert(len > 0); + physical_fit = gistfitpage(itup, len, 0); + + /* + * Fillfactor is a soft limit. Allow a physically valid page to exceed it + * when splitting would necessarily produce a singleton page. + */ + if (freespace > 0 && physical_fit && len < 4) + return gistnosplit(r, itup, len, giststate); /* * If a single tuple doesn't fit on a page, no amount of splitting will @@ -1482,6 +1509,11 @@ gistSplit(Relation r, sizeof(bool) * giststate->nonLeafTupdesc->natts); gistSplitByKey(r, page, itup, len, giststate, &v, 0); + /* Do not create a singleton page solely to satisfy fillfactor. */ + if (freespace > 0 && physical_fit && + (v.splitVector.spl_nleft < 2 || v.splitVector.spl_nright < 2)) + return gistnosplit(r, itup, len, giststate); + /* form left and right vector */ lvectup = palloc_array(IndexTuple, len + 1); rvectup = palloc_array(IndexTuple, len + 1); @@ -1493,9 +1525,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, + giststate, freespace); } else { @@ -1505,12 +1538,14 @@ 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)) { SplitPageLayout *resptr, *subres; - resptr = subres = gistSplit(r, page, lvectup, v.splitVector.spl_nleft, giststate); + resptr = subres = gistSplit(r, page, lvectup, + v.splitVector.spl_nleft, + giststate, freespace); /* install on list's tail */ while (resptr->next) diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c index 7f57c787f4c..587af3c1da7 100644 --- a/src/backend/access/gist/gistbuild.c +++ b/src/backend/access/gist/gistbuild.c @@ -464,8 +464,8 @@ gist_indexsortbuild_levelstate_add(GISTBuildState *state, { Size sizeNeeded; - /* Check if tuple can be added to the current page */ - sizeNeeded = IndexTupleSize(itup) + sizeof(ItemIdData); /* fillfactor ignored */ + /* Check if tuple can be added to the current page. */ + sizeNeeded = IndexTupleSize(itup) + sizeof(ItemIdData); if (PageGetFreeSpace(levelstate->pages[levelstate->current_page]) < sizeNeeded) { Page newPage; @@ -520,8 +520,15 @@ gist_indexsortbuild_levelstate_flush(GISTBuildState *state, pfree(itvec_local); } - /* Apply picksplit to list of all collected tuples */ - dist = gistSplit(state->indexrel, levelstate->pages[0], itvec, vect_len, state->giststate); + /* Apply picksplit to list of all collected tuples. */ + dist = gistSplit(state->indexrel, levelstate->pages[0], itvec, + vect_len, state->giststate, state->freespace); + } + else if (!gistfitpage(itvec, vect_len, state->freespace)) + { + /* Split a single buffered page if it exceeds fillfactor. */ + dist = gistSplit(state->indexrel, levelstate->pages[0], itvec, + vect_len, state->giststate, state->freespace); } else { diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c index 0f58f61879f..bb16e845128 100644 --- a/src/backend/access/gist/gistutil.c +++ b/src/backend/access/gist/gistutil.c @@ -76,15 +76,14 @@ gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete, Size f } bool -gistfitpage(IndexTuple *itvec, int len) +gistfitpage(IndexTuple *itvec, int len, Size freespace) { int i; - Size size = 0; + Size size = freespace; 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 44514f1cb8d..b13fb48571e 100644 --- a/src/include/access/gist_private.h +++ b/src/include/access/gist_private.h @@ -433,7 +433,8 @@ extern bool gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate, bool is_build); extern SplitPageLayout *gistSplit(Relation r, Page page, IndexTuple *itup, - int len, GISTSTATE *giststate); + int len, GISTSTATE *giststate, + Size freespace); /* gistxlog.c */ extern XLogRecPtr gistXLogPageDelete(Buffer buffer, @@ -481,7 +482,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, Size 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, Relation heaprel); diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out index ae5b522b3c6..f97ecd7c035 100644 --- a/src/test/regress/expected/gist.out +++ b/src/test/regress/expected/gist.out @@ -25,6 +25,27 @@ insert into gist_point_tbl (id, p) select g, point(g*10, g*10) from generate_series(1, 10000) g; insert into gist_point_tbl (id, p) select g+100000, point(g*10+1, g*10+1) from generate_series(1, 10000) g; +-- Check that sorted builds honor fillfactor +create index gist_pointidx_ff90 on gist_point_tbl using gist(p) with (fillfactor = 90); +create index gist_pointidx_ff40 on gist_point_tbl using gist(p) with (fillfactor = 40); +select pg_relation_size('gist_pointidx_ff40') > + pg_relation_size('gist_pointidx_ff90') as fillfactor_honored; + fillfactor_honored +-------------------- + t +(1 row) + +drop index gist_pointidx_ff90, gist_pointidx_ff40; +-- Fillfactor must not force pages with fewer than two tuples +create table gist_point_tbl_big(p point, payload text); +insert into gist_point_tbl_big +select point(g, g), string_agg(md5((g * 1000 + i)::text), '') +from generate_series(1, 20) g, + generate_series(1, 40) i +group by g; +create index gist_pointidx_big on gist_point_tbl_big using gist(p) + include (payload) with (fillfactor = 10); +drop table gist_point_tbl_big; -- To test vacuum, delete some entries from all over the index. delete from gist_point_tbl where id % 2 = 1; -- And also delete some concentration of values. diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql index 1ebb1d9ee43..679a067d8ff 100644 --- a/src/test/regress/sql/gist.sql +++ b/src/test/regress/sql/gist.sql @@ -25,6 +25,24 @@ select g, point(g*10, g*10) from generate_series(1, 10000) g; insert into gist_point_tbl (id, p) select g+100000, point(g*10+1, g*10+1) from generate_series(1, 10000) g; +-- Check that sorted builds honor fillfactor +create index gist_pointidx_ff90 on gist_point_tbl using gist(p) with (fillfactor = 90); +create index gist_pointidx_ff40 on gist_point_tbl using gist(p) with (fillfactor = 40); +select pg_relation_size('gist_pointidx_ff40') > + pg_relation_size('gist_pointidx_ff90') as fillfactor_honored; +drop index gist_pointidx_ff90, gist_pointidx_ff40; + +-- Fillfactor must not force pages with fewer than two tuples +create table gist_point_tbl_big(p point, payload text); +insert into gist_point_tbl_big +select point(g, g), string_agg(md5((g * 1000 + i)::text), '') +from generate_series(1, 20) g, + generate_series(1, 40) i +group by g; +create index gist_pointidx_big on gist_point_tbl_big using gist(p) + include (payload) with (fillfactor = 10); +drop table gist_point_tbl_big; + -- To test vacuum, delete some entries from all over the index. delete from gist_point_tbl where id % 2 = 1; -- 2.50.1 (Apple Git-155)