From 9f376e146b2f1fe1bc4d07380f2a047d5c375581 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 2 Nov 2010 17:15:07 -0400 Subject: [PATCH] Ensure an index that uses a whole-row Var still depends on its table. We failed to record any dependency on the underlying table for an index declared like "create index i on t (foo(t.*))". This would create trouble if the table were dropped without previously dropping the index. To fix, simplify some overly-cute code in index_create(), accepting the possibility that sometimes the whole-table dependency will be redundant. Also document this hazard in dependency.c. Per report from Kevin Grittner. In passing, prevent a core dump in pg_get_indexdef() if the index's table can't be found. I came across this while experimenting with Kevin's example. Not sure it's a real issue when the catalogs aren't corrupt, but might as well be cautious. Back-patch to all supported versions. --- src/backend/catalog/dependency.c | 15 ++++++++++++++- src/backend/catalog/index.c | 15 +++++---------- src/backend/utils/adt/ruleutils.c | 24 +++++++++++++++++++++--- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index 18e07eb956..bb14a438b0 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -1238,6 +1238,12 @@ recordDependencyOnExpr(const ObjectAddress *depender, * range table. An additional frammish is that dependencies on that * relation (or its component columns) will be marked with 'self_behavior', * whereas 'behavior' is used for everything else. + * + * NOTE: the caller should ensure that a whole-table dependency on the + * specified relation is created separately, if one is needed. In particular, + * a whole-row Var "relation.*" will not cause this routine to emit any + * dependency item. This is appropriate behavior for subexpressions of an + * ordinary query, so other cases need to cope as necessary. */ void recordDependencyOnSingleRelExpr(const ObjectAddress *depender, @@ -1350,7 +1356,14 @@ find_expr_references_walker(Node *node, /* * A whole-row Var references no specific columns, so adds no new - * dependency. + * dependency. (We assume that there is a whole-table dependency + * arising from each underlying rangetable entry. While we could + * record such a dependency when finding a whole-row Var that + * references a relation directly, it's quite unclear how to extend + * that to whole-row Vars for JOINs, so it seems better to leave the + * responsibility with the range table. Note that this poses some + * risks for identifying dependencies of stand-alone expressions: + * whole-table references may need to be created separately.) */ if (var->varattno == InvalidAttrNumber) return false; diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index b82b9b66fe..b437c9976a 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -50,7 +50,6 @@ #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" #include "optimizer/clauses.h" -#include "optimizer/var.h" #include "parser/parser.h" #include "storage/bufmgr.h" #include "storage/lmgr.h" @@ -858,16 +857,12 @@ index_create(Oid heapRelationId, } /* - * It's possible for an index to not depend on any columns of the - * table at all, in which case we need to give it a dependency on - * the table as a whole; else it won't get dropped when the table - * is dropped. This edge case is not totally useless; for - * example, a unique index on a constant expression can serve to - * prevent a table from containing more than one row. + * If there are no simply-referenced columns, give the index an + * auto dependency on the whole table. In most cases, this will + * be redundant, but it might not be if the index expressions and + * predicate contain no Vars or only whole-row Vars. */ - if (!have_simple_col && - !contain_vars_of_level((Node *) indexInfo->ii_Expressions, 0) && - !contain_vars_of_level((Node *) indexInfo->ii_Predicate, 0)) + if (!have_simple_col) { referenced.classId = RelationRelationId; referenced.objectId = heapRelationId; diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index a5cc48b47c..dc17685563 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -240,6 +240,7 @@ static void get_opclass_name(Oid opclass, Oid actual_datatype, static Node *processIndirection(Node *node, deparse_context *context, bool printit); static void printSubscripts(ArrayRef *aref, deparse_context *context); +static char *get_relation_name(Oid relid); static char *generate_relation_name(Oid relid, List *namespaces); static char *generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes, bool *is_variadic); @@ -857,7 +858,7 @@ pg_get_indexdef_worker(Oid indexrelid, int colno, indexpr_item = list_head(indexprs); - context = deparse_context_for(get_rel_name(indrelid), indrelid); + context = deparse_context_for(get_relation_name(indrelid), indrelid); /* * Start the index definition. Note that the index's name should never be @@ -1261,7 +1262,7 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand, if (conForm->conrelid != InvalidOid) { /* relation constraint */ - context = deparse_context_for(get_rel_name(conForm->conrelid), + context = deparse_context_for(get_relation_name(conForm->conrelid), conForm->conrelid); } else @@ -6314,7 +6315,7 @@ get_from_clause_item(Node *jtnode, Query *query, deparse_context *context) gavealias = true; } else if (rte->rtekind == RTE_RELATION && - strcmp(rte->eref->aliasname, get_rel_name(rte->relid)) != 0) + strcmp(rte->eref->aliasname, get_relation_name(rte->relid)) != 0) { /* * Apparently the rel has been renamed since the rule was made. @@ -6817,6 +6818,23 @@ quote_qualified_identifier(const char *qualifier, return buf.data; } +/* + * get_relation_name + * Get the unqualified name of a relation specified by OID + * + * This differs from the underlying get_rel_name() function in that it will + * throw error instead of silently returning NULL if the OID is bad. + */ +static char * +get_relation_name(Oid relid) +{ + char *relname = get_rel_name(relid); + + if (!relname) + elog(ERROR, "cache lookup failed for relation %u", relid); + return relname; +} + /* * generate_relation_name * Compute the name to display for a relation specified by OID