From 25c7183c06271b44ad2f13fc6ffe435544bee970 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 1 Dec 2019 13:09:27 -0500 Subject: [PATCH] Fix misbehavior with expression indexes on ON COMMIT DELETE ROWS tables. We implement ON COMMIT DELETE ROWS by truncating tables marked that way, which requires also truncating/rebuilding their indexes. But RelationTruncateIndexes asks the relcache for up-to-date copies of any index expressions, which may cause execution of eval_const_expressions on them, which can result in actual execution of subexpressions. This is a bad thing to have happening during ON COMMIT. Manuel Rigger reported that use of a SQL function resulted in crashes due to expectations that ActiveSnapshot would be set, which it isn't. The most obvious fix perhaps would be to push a snapshot during PreCommit_on_commit_actions, but I think that would just open the door to more problems: CommitTransaction explicitly expects that no user-defined code can be running at this point. Fortunately, since we know that no tuples exist to be indexed, there seems no need to use the real index expressions or predicates during RelationTruncateIndexes. We can set up dummy index expressions instead (we do need something that will expose the right data type, as there are places that build index tupdescs based on this), and just ignore predicates and exclusion constraints. In a green field it'd likely be better to reimplement ON COMMIT DELETE ROWS using the same "init fork" infrastructure used for unlogged relations. That seems impractical without catalog changes though, and even without that it'd be too big a change to back-patch. So for now do it like this. Per private report from Manuel Rigger. This has been broken forever, so back-patch to all supported branches. --- src/backend/catalog/heap.c | 11 +++++- src/backend/catalog/index.c | 63 ++++++++++++++++++++++++++++++ src/backend/utils/cache/relcache.c | 52 ++++++++++++++++++++++++ src/include/catalog/index.h | 2 + src/include/utils/relcache.h | 1 + src/test/regress/expected/temp.out | 2 + src/test/regress/sql/temp.sql | 3 ++ 7 files changed, 132 insertions(+), 2 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index ac9df9eefe7..4d61e8e071c 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2795,8 +2795,15 @@ RelationTruncateIndexes(Relation heapRelation) /* Open the index relation; use exclusive lock, just to be sure */ currentIndex = index_open(indexId, AccessExclusiveLock); - /* Fetch info needed for index_build */ - indexInfo = BuildIndexInfo(currentIndex); + /* + * Fetch info needed for index_build. Since we know there are no + * tuples that actually need indexing, we can use a dummy IndexInfo. + * This is slightly cheaper to build, but the real point is to avoid + * possibly running user-defined code in index expressions or + * predicates. We might be getting invoked during ON COMMIT + * processing, and we don't want to run any such code then. + */ + indexInfo = BuildDummyIndexInfo(currentIndex); /* * Now truncate the actual file (and discard buffers). diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index a6b56d31007..942d43db892 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1706,6 +1706,69 @@ BuildIndexInfo(Relation index) return ii; } +/* ---------------- + * BuildDummyIndexInfo + * Construct a dummy IndexInfo record for an open index + * + * This differs from the real BuildIndexInfo in that it will never run any + * user-defined code that might exist in index expressions or predicates. + * Instead of the real index expressions, we return null constants that have + * the right types/typmods/collations. Predicates and exclusion clauses are + * just ignored. This is sufficient for the purpose of truncating an index, + * since we will not need to actually evaluate the expressions or predicates; + * the only thing that's likely to be done with the data is construction of + * a tupdesc describing the index's rowtype. + * ---------------- + */ +IndexInfo * +BuildDummyIndexInfo(Relation index) +{ + IndexInfo *ii = makeNode(IndexInfo); + Form_pg_index indexStruct = index->rd_index; + int i; + int numKeys; + + /* check the number of keys, and copy attr numbers into the IndexInfo */ + numKeys = indexStruct->indnatts; + if (numKeys < 1 || numKeys > INDEX_MAX_KEYS) + elog(ERROR, "invalid indnatts %d for index %u", + numKeys, RelationGetRelid(index)); + ii->ii_NumIndexAttrs = numKeys; + for (i = 0; i < numKeys; i++) + ii->ii_KeyAttrNumbers[i] = indexStruct->indkey.values[i]; + + /* fetch dummy expressions for expressional indexes */ + ii->ii_Expressions = RelationGetDummyIndexExpressions(index); + ii->ii_ExpressionsState = NIL; + + /* pretend there is no predicate */ + ii->ii_Predicate = NIL; + ii->ii_PredicateState = NULL; + + /* We ignore the exclusion constraint if any */ + ii->ii_ExclusionOps = NULL; + ii->ii_ExclusionProcs = NULL; + ii->ii_ExclusionStrats = NULL; + + /* other info */ + ii->ii_Unique = indexStruct->indisunique; + ii->ii_ReadyForInserts = IndexIsReady(indexStruct); + /* assume not doing speculative insertion for now */ + ii->ii_UniqueOps = NULL; + ii->ii_UniqueProcs = NULL; + ii->ii_UniqueStrats = NULL; + + /* initialize index-build state to default */ + ii->ii_Concurrent = false; + ii->ii_BrokenHotChain = false; + + /* set up for possible use by index AM */ + ii->ii_AmCache = NULL; + ii->ii_Context = CurrentMemoryContext; + + return ii; +} + /* ---------------- * BuildSpeculativeIndexInfo * Add extra state to IndexInfo record diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 24e830a39c1..b6720184633 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -65,6 +65,7 @@ #include "commands/policy.h" #include "commands/trigger.h" #include "miscadmin.h" +#include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" #include "optimizer/clauses.h" #include "optimizer/prep.h" @@ -4816,6 +4817,57 @@ RelationGetIndexExpressions(Relation relation) return result; } +/* + * RelationGetDummyIndexExpressions -- get dummy expressions for an index + * + * Return a list of dummy expressions (just Const nodes) with the same + * types/typmods/collations as the index's real expressions. This is + * useful in situations where we don't want to run any user-defined code. + */ +List * +RelationGetDummyIndexExpressions(Relation relation) +{ + List *result; + Datum exprsDatum; + bool isnull; + char *exprsString; + List *rawExprs; + ListCell *lc; + + /* Quick exit if there is nothing to do. */ + if (relation->rd_indextuple == NULL || + heap_attisnull(relation->rd_indextuple, Anum_pg_index_indexprs)) + return NIL; + + /* Extract raw node tree(s) from index tuple. */ + exprsDatum = heap_getattr(relation->rd_indextuple, + Anum_pg_index_indexprs, + GetPgIndexDescriptor(), + &isnull); + Assert(!isnull); + exprsString = TextDatumGetCString(exprsDatum); + rawExprs = (List *) stringToNode(exprsString); + pfree(exprsString); + + /* Construct null Consts; the typlen and typbyval are arbitrary. */ + result = NIL; + foreach(lc, rawExprs) + { + Node *rawExpr = (Node *) lfirst(lc); + + result = lappend(result, + makeConst(exprType(rawExpr), + exprTypmod(rawExpr), + exprCollation(rawExpr), + 1, + (Datum) 0, + true, + true)); + } + + return result; +} + /* * RelationGetIndexPredicate -- get the index predicate for an index * diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index ebca7740eae..7c56182c723 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -82,6 +82,8 @@ extern void index_drop(Oid indexId, bool concurrent); extern IndexInfo *BuildIndexInfo(Relation index); +extern IndexInfo *BuildDummyIndexInfo(Relation index); + extern void BuildSpeculativeIndexInfo(Relation index, IndexInfo *ii); extern void FormIndexDatum(IndexInfo *indexInfo, diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h index 3c53cefe4b4..e2207ccd3d0 100644 --- a/src/include/utils/relcache.h +++ b/src/include/utils/relcache.h @@ -44,6 +44,7 @@ extern Oid RelationGetOidIndex(Relation relation); extern Oid RelationGetPrimaryKeyIndex(Relation relation); extern Oid RelationGetReplicaIndex(Relation relation); extern List *RelationGetIndexExpressions(Relation relation); +extern List *RelationGetDummyIndexExpressions(Relation relation); extern List *RelationGetIndexPredicate(Relation relation); typedef enum IndexAttrBitmapKind diff --git a/src/test/regress/expected/temp.out b/src/test/regress/expected/temp.out index 5e089ef0503..47d58e829a3 100644 --- a/src/test/regress/expected/temp.out +++ b/src/test/regress/expected/temp.out @@ -49,6 +49,8 @@ LINE 1: SELECT * FROM temptest; ^ -- Test ON COMMIT DELETE ROWS CREATE TEMP TABLE temptest(col int) ON COMMIT DELETE ROWS; +-- while we're here, verify successful truncation of index with SQL function +CREATE INDEX ON temptest(bit_length('')); BEGIN; INSERT INTO temptest VALUES (1); INSERT INTO temptest VALUES (2); diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql index f8d2a285c3c..e72618fa7f7 100644 --- a/src/test/regress/sql/temp.sql +++ b/src/test/regress/sql/temp.sql @@ -55,6 +55,9 @@ SELECT * FROM temptest; CREATE TEMP TABLE temptest(col int) ON COMMIT DELETE ROWS; +-- while we're here, verify successful truncation of index with SQL function +CREATE INDEX ON temptest(bit_length('')); + BEGIN; INSERT INTO temptest VALUES (1); INSERT INTO temptest VALUES (2);