diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c index 88ae2499dd..520983ede1 100644 --- a/src/backend/access/spgist/spgdoinsert.c +++ b/src/backend/access/spgist/spgdoinsert.c @@ -669,7 +669,8 @@ checkAllTheSame(spgPickSplitIn *in, spgPickSplitOut *out, bool tooBig, * will eventually terminate if lack of balance is the issue. If the tuple * is too big, we assume that repeated picksplit operations will eventually * make it small enough by repeated prefix-stripping. A broken opclass could - * make this an infinite loop, though. + * make this an infinite loop, though, so spgdoinsert() checks that the + * leaf datums get smaller each time. */ static bool doPickSplit(Relation index, SpGistState *state, @@ -1918,6 +1919,8 @@ spgdoinsert(Relation index, SpGistState *state, int level = 0; Datum leafDatums[INDEX_MAX_KEYS]; int leafSize; + int prevLeafSize; + int numNoProgressCycles = 0; SPPageDesc current, parent; FmgrInfo *procinfo = NULL; @@ -1988,9 +1991,10 @@ spgdoinsert(Relation index, SpGistState *state, /* * If it isn't gonna fit, and the opclass can't reduce the datum size by - * suffixing, bail out now rather than getting into an endless loop. + * suffixing, bail out now rather than doing a lot of useless work. */ - if (leafSize > SPGIST_PAGE_CAPACITY && !state->config.longValuesOK) + if (leafSize > SPGIST_PAGE_CAPACITY && + (isnull || !state->config.longValuesOK)) ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("index row size %zu exceeds maximum %zu for index \"%s\"", @@ -2218,6 +2222,7 @@ spgdoinsert(Relation index, SpGistState *state, /* Adjust level as per opclass request */ level += out.result.matchNode.levelAdd; /* Replace leafDatum and recompute leafSize */ + prevLeafSize = leafSize; if (!isnull) { leafDatums[spgKeyColumn] = out.result.matchNode.restDatum; @@ -2226,19 +2231,50 @@ spgdoinsert(Relation index, SpGistState *state, leafSize += sizeof(ItemIdData); } + /* + * Check new tuple size; fail if it can't fit, unless the + * opclass says it can handle the situation by suffixing. + * In that case, prevent an infinite loop by checking to + * see if we are actually making progress by reducing the + * leafSize size in each pass. This is a bit tricky + * though. Because of alignment considerations, the total + * tuple size might not decrease on every pass. Also, + * there are edge cases where the choose method might seem + * to not make progress for a cycle or two. Somewhat + * arbitrarily, we allow up to 10 no-progress iterations + * before failing. (The limit should be more than + * MAXALIGN, to accommodate opclasses that trim one byte + * from the leaf datum per pass.) + */ + if (leafSize > SPGIST_PAGE_CAPACITY) + { + bool ok = false; + + if (state->config.longValuesOK && !isnull) + { + if (leafSize < prevLeafSize) + { + ok = true; + numNoProgressCycles = 0; + } + else if (++numNoProgressCycles < 10) + ok = true; + } + if (!ok) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("index row size %zu exceeds maximum %zu for index \"%s\"", + leafSize - sizeof(ItemIdData), + SPGIST_PAGE_CAPACITY - sizeof(ItemIdData), + RelationGetRelationName(index)), + errhint("Values larger than a buffer page cannot be indexed."))); + } + /* * Loop around and attempt to insert the new leafDatum at * "current" (which might reference an existing child * tuple, or might be invalid to force us to find a new * page for the tuple). - * - * Note: if the opclass sets longValuesOK, we rely on the - * choose function to eventually shorten the leafDatum - * enough to fit on a page. We could add a test here to - * complain if the datum doesn't get visibly shorter each - * time, but that could get in the way of opclasses that - * "simplify" datums in a way that doesn't necessarily - * lead to physical shortening on every cycle. */ break; case spgAddNode: diff --git a/src/test/modules/spgist_name_ops/expected/spgist_name_ops.out b/src/test/modules/spgist_name_ops/expected/spgist_name_ops.out index 0ac99d08fb..ac0ddcecea 100644 --- a/src/test/modules/spgist_name_ops/expected/spgist_name_ops.out +++ b/src/test/modules/spgist_name_ops/expected/spgist_name_ops.out @@ -61,6 +61,12 @@ select * from t binary_upgrade_set_next_toast_pg_class_oid | 1 | binary_upgrade_set_next_toast_pg_class_oid (9 rows) +-- Verify clean failure when INCLUDE'd columns result in overlength tuple +-- The error message details are platform-dependent, so show only SQLSTATE +\set VERBOSITY sqlstate +insert into t values(repeat('xyzzy', 12), 42, repeat('xyzzy', 4000)); +ERROR: 54000 +\set VERBOSITY default drop index t_f1_f2_f3_idx; create index on t using spgist(f1 name_ops_old) include(f2, f3); \d+ t_f1_f2_f3_idx @@ -100,3 +106,7 @@ select * from t binary_upgrade_set_next_toast_pg_class_oid | 1 | binary_upgrade_set_next_toast_pg_class_oid (9 rows) +\set VERBOSITY sqlstate +insert into t values(repeat('xyzzy', 12), 42, repeat('xyzzy', 4000)); +ERROR: 54000 +\set VERBOSITY default diff --git a/src/test/modules/spgist_name_ops/sql/spgist_name_ops.sql b/src/test/modules/spgist_name_ops/sql/spgist_name_ops.sql index 76e78ba41c..982f221a8b 100644 --- a/src/test/modules/spgist_name_ops/sql/spgist_name_ops.sql +++ b/src/test/modules/spgist_name_ops/sql/spgist_name_ops.sql @@ -27,6 +27,12 @@ select * from t where f1 > 'binary_upgrade_set_n' and f1 < 'binary_upgrade_set_p' order by 1; +-- Verify clean failure when INCLUDE'd columns result in overlength tuple +-- The error message details are platform-dependent, so show only SQLSTATE +\set VERBOSITY sqlstate +insert into t values(repeat('xyzzy', 12), 42, repeat('xyzzy', 4000)); +\set VERBOSITY default + drop index t_f1_f2_f3_idx; create index on t using spgist(f1 name_ops_old) include(f2, f3); @@ -39,3 +45,7 @@ select * from t select * from t where f1 > 'binary_upgrade_set_n' and f1 < 'binary_upgrade_set_p' order by 1; + +\set VERBOSITY sqlstate +insert into t values(repeat('xyzzy', 12), 42, repeat('xyzzy', 4000)); +\set VERBOSITY default