From 6a75562ed16b5fa52cfd8830e4546972e647db26 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 22 Feb 2015 13:59:09 -0500 Subject: [PATCH] Get rid of multiple applications of transformExpr() to the same tree. transformExpr() has for many years had provisions to do nothing when applied to an already-transformed expression tree. However, this was always ugly and of dubious reliability, so we'd be much better off without it. The primary historical reason for it was that gram.y sometimes returned multiple links to the same subexpression, which is no longer true as of my BETWEEN fixes. We'd also grown some lazy hacks in CREATE TABLE LIKE (failing to distinguish between raw and already-transformed index specifications) and one or two other places. This patch removes the need for and support for re-transforming already transformed expressions. The index case is dealt with by adding a flag to struct IndexStmt to indicate that it's already been transformed; which has some benefit anyway in that tablecmds.c can now Assert that transformation has happened rather than just assuming. The other main reason was some rather sloppy code for array type coercion, which can be fixed (and its performance improved too) by refactoring. I did leave transformJoinUsingClause() still constructing expressions containing untransformed operator nodes being applied to Vars, so that transformExpr() still has to allow Var inputs. But that's a much narrower, and safer, special case than before, since Vars will never appear in a raw parse tree, and they don't have any substructure to worry about. In passing fix some oversights in the patch that added CREATE INDEX IF NOT EXISTS (missing processing of IndexStmt.if_not_exists). These appear relatively harmless, but still sloppy coding practice. --- src/backend/bootstrap/bootparse.y | 4 + src/backend/commands/tablecmds.c | 5 +- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c | 1 + src/backend/nodes/outfuncs.c | 2 + src/backend/parser/gram.y | 2 + src/backend/parser/parse_clause.c | 9 +- src/backend/parser/parse_expr.c | 163 ++++++++++------------------- src/backend/parser/parse_utilcmd.c | 11 ++ src/include/nodes/parsenodes.h | 1 + 10 files changed, 83 insertions(+), 116 deletions(-) diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y index 9edd1a0aff..fdb1f7faac 100644 --- a/src/backend/bootstrap/bootparse.y +++ b/src/backend/bootstrap/bootparse.y @@ -304,7 +304,9 @@ Boot_DeclareIndexStmt: stmt->isconstraint = false; stmt->deferrable = false; stmt->initdeferred = false; + stmt->transformed = false; stmt->concurrent = false; + stmt->if_not_exists = false; /* locks and races need not concern us in bootstrap mode */ relationId = RangeVarGetRelid(stmt->relation, NoLock, @@ -345,7 +347,9 @@ Boot_DeclareUniqueIndexStmt: stmt->isconstraint = false; stmt->deferrable = false; stmt->initdeferred = false; + stmt->transformed = false; stmt->concurrent = false; + stmt->if_not_exists = false; /* locks and races need not concern us in bootstrap mode */ relationId = RangeVarGetRelid(stmt->relation, NoLock, diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b2993b80fb..f5d5b6302f 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5705,6 +5705,9 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel, Assert(IsA(stmt, IndexStmt)); Assert(!stmt->concurrent); + /* The IndexStmt has already been through transformIndexStmt */ + Assert(stmt->transformed); + /* suppress schema rights check when rebuilding existing index */ check_rights = !is_rebuild; /* skip index build if phase 3 will do it or we're reusing an old one */ @@ -5712,8 +5715,6 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel, /* suppress notices when rebuilding existing index */ quiet = is_rebuild; - /* The IndexStmt has already been through transformIndexStmt */ - new_index = DefineIndex(RelationGetRelid(rel), stmt, InvalidOid, /* no predefined OID */ diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index e5b0dce477..8798cbe43d 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -2937,6 +2937,7 @@ _copyIndexStmt(const IndexStmt *from) COPY_SCALAR_FIELD(isconstraint); COPY_SCALAR_FIELD(deferrable); COPY_SCALAR_FIELD(initdeferred); + COPY_SCALAR_FIELD(transformed); COPY_SCALAR_FIELD(concurrent); COPY_SCALAR_FIELD(if_not_exists); diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index 6e8b308a3e..a90386b800 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -1209,6 +1209,7 @@ _equalIndexStmt(const IndexStmt *a, const IndexStmt *b) COMPARE_SCALAR_FIELD(isconstraint); COMPARE_SCALAR_FIELD(deferrable); COMPARE_SCALAR_FIELD(initdeferred); + COMPARE_SCALAR_FIELD(transformed); COMPARE_SCALAR_FIELD(concurrent); COMPARE_SCALAR_FIELD(if_not_exists); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index d8e2077d60..67b7d40406 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -2078,7 +2078,9 @@ _outIndexStmt(StringInfo str, const IndexStmt *node) WRITE_BOOL_FIELD(isconstraint); WRITE_BOOL_FIELD(deferrable); WRITE_BOOL_FIELD(initdeferred); + WRITE_BOOL_FIELD(transformed); WRITE_BOOL_FIELD(concurrent); + WRITE_BOOL_FIELD(if_not_exists); } static void diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 67c7c84dee..b8af296397 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -6563,6 +6563,7 @@ IndexStmt: CREATE opt_unique INDEX opt_concurrently opt_index_name n->isconstraint = false; n->deferrable = false; n->initdeferred = false; + n->transformed = false; n->if_not_exists = false; $$ = (Node *)n; } @@ -6588,6 +6589,7 @@ IndexStmt: CREATE opt_unique INDEX opt_concurrently opt_index_name n->isconstraint = false; n->deferrable = false; n->initdeferred = false; + n->transformed = false; n->if_not_exists = true; $$ = (Node *)n; } diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index 654dce6755..8d90b5098a 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -339,10 +339,11 @@ transformJoinUsingClause(ParseState *pstate, /* * We cheat a little bit here by building an untransformed operator tree - * whose leaves are the already-transformed Vars. This is OK because - * transformExpr() won't complain about already-transformed subnodes. - * However, this does mean that we have to mark the columns as requiring - * SELECT privilege for ourselves; transformExpr() won't do it. + * whose leaves are the already-transformed Vars. This requires collusion + * from transformExpr(), which normally could be expected to complain + * about already-transformed subnodes. However, this does mean that we + * have to mark the columns as requiring SELECT privilege for ourselves; + * transformExpr() won't do it. */ forboth(lvars, leftVars, rvars, rightVars) { diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 67a2310ad0..791639a7db 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -81,28 +81,8 @@ static Expr *make_distinct_op(ParseState *pstate, List *opname, /* * transformExpr - * Analyze and transform expressions. Type checking and type casting is - * done here. The optimizer and the executor cannot handle the original - * (raw) expressions collected by the parse tree. Hence the transformation - * here. - * - * NOTE: there are various cases in which this routine will get applied to - * an already-transformed expression. Some examples: - * 1. At least one construct (BETWEEN/AND) puts the same nodes - * into two branches of the parse tree; hence, some nodes - * are transformed twice. - * 2. Another way it can happen is that coercion of an operator or - * function argument to the required type (via coerce_type()) - * can apply transformExpr to an already-transformed subexpression. - * An example here is "SELECT count(*) + 1.0 FROM table". - * 3. CREATE TABLE t1 (LIKE t2 INCLUDING INDEXES) can pass in - * already-transformed index expressions. - * While it might be possible to eliminate these cases, the path of - * least resistance so far has been to ensure that transformExpr() does - * no damage if applied to an already-transformed tree. This is pretty - * easy for cases where the transformation replaces one node type with - * another, such as A_Const => Const; we just do nothing when handed - * a Const. More care is needed for node types that are used as both - * input and output of transformExpr; see SubLink for example. + * done here. This processing converts the raw grammar output into + * expression trees with fully determined semantics. */ Node * transformExpr(ParseState *pstate, Node *expr, ParseExprKind exprKind) @@ -168,48 +148,8 @@ transformExprRecurse(ParseState *pstate, Node *expr) break; case T_TypeCast: - { - TypeCast *tc = (TypeCast *) expr; - - /* - * If the subject of the typecast is an ARRAY[] construct and - * the target type is an array type, we invoke - * transformArrayExpr() directly so that we can pass down the - * type information. This avoids some cases where - * transformArrayExpr() might not infer the correct type. - */ - if (IsA(tc->arg, A_ArrayExpr)) - { - Oid targetType; - Oid elementType; - int32 targetTypmod; - - typenameTypeIdAndMod(pstate, tc->typeName, - &targetType, &targetTypmod); - - /* - * If target is a domain over array, work with the base - * array type here. transformTypeCast below will cast the - * array type to the domain. In the usual case that the - * target is not a domain, transformTypeCast is a no-op. - */ - targetType = getBaseTypeAndTypmod(targetType, - &targetTypmod); - elementType = get_element_type(targetType); - if (OidIsValid(elementType)) - { - tc = copyObject(tc); - tc->arg = transformArrayExpr(pstate, - (A_ArrayExpr *) tc->arg, - targetType, - elementType, - targetTypmod); - } - } - - result = transformTypeCast(pstate, tc); - break; - } + result = transformTypeCast(pstate, (TypeCast *) expr); + break; case T_CollateClause: result = transformCollateClause(pstate, (CollateClause *) expr); @@ -324,37 +264,19 @@ transformExprRecurse(ParseState *pstate, Node *expr) result = transformCurrentOfExpr(pstate, (CurrentOfExpr *) expr); break; - /********************************************* - * Quietly accept node types that may be presented when we are - * called on an already-transformed tree. + /* + * CaseTestExpr and SetToDefault don't require any processing; + * they are only injected into parse trees in fully-formed state. * - * Do any other node types need to be accepted? For now we are - * taking a conservative approach, and only accepting node - * types that are demonstrably necessary to accept. - *********************************************/ - case T_Var: - case T_Const: - case T_Param: - case T_Aggref: - case T_WindowFunc: - case T_ArrayRef: - case T_FuncExpr: - case T_OpExpr: - case T_DistinctExpr: - case T_NullIfExpr: - case T_ScalarArrayOpExpr: - case T_FieldSelect: - case T_FieldStore: - case T_RelabelType: - case T_CoerceViaIO: - case T_ArrayCoerceExpr: - case T_ConvertRowtypeExpr: - case T_CollateExpr: + * Ordinarily we should not see a Var here, but it is convenient + * for transformJoinUsingClause() to create untransformed operator + * trees containing already-transformed Vars. The best + * alternative would be to deconstruct and reconstruct column + * references, which seems expensively pointless. So allow it. + */ case T_CaseTestExpr: - case T_ArrayExpr: - case T_CoerceToDomain: - case T_CoerceToDomainValue: case T_SetToDefault: + case T_Var: { result = (Node *) expr; break; @@ -1461,10 +1383,6 @@ transformCaseExpr(ParseState *pstate, CaseExpr *c) Node *defresult; Oid ptype; - /* If we already transformed this node, do nothing */ - if (OidIsValid(c->casetype)) - return (Node *) c; - newc = makeNode(CaseExpr); /* transform the test expression, if any */ @@ -1592,10 +1510,6 @@ transformSubLink(ParseState *pstate, SubLink *sublink) Query *qtree; const char *err; - /* If we already transformed this node, do nothing */ - if (IsA(sublink->subselect, Query)) - return result; - /* * Check to see if the sublink is in an invalid place within the query. We * allow sublinks everywhere in SELECT/INSERT/UPDATE/DELETE, but generally @@ -1964,10 +1878,6 @@ transformRowExpr(ParseState *pstate, RowExpr *r) int fnum; ListCell *lc; - /* If we already transformed this node, do nothing */ - if (OidIsValid(r->row_typeid)) - return (Node *) r; - newr = makeNode(RowExpr); /* Transform the field expressions */ @@ -2074,10 +1984,6 @@ transformXmlExpr(ParseState *pstate, XmlExpr *x) ListCell *lc; int i; - /* If we already transformed this node, do nothing */ - if (OidIsValid(x->type)) - return (Node *) x; - newx = makeNode(XmlExpr); newx->op = x->op; if (x->name) @@ -2381,14 +2287,51 @@ static Node * transformTypeCast(ParseState *pstate, TypeCast *tc) { Node *result; - Node *expr = transformExprRecurse(pstate, tc->arg); - Oid inputType = exprType(expr); + Node *expr; + Oid inputType; Oid targetType; int32 targetTypmod; int location; typenameTypeIdAndMod(pstate, tc->typeName, &targetType, &targetTypmod); + /* + * If the subject of the typecast is an ARRAY[] construct and the target + * type is an array type, we invoke transformArrayExpr() directly so that + * we can pass down the type information. This avoids some cases where + * transformArrayExpr() might not infer the correct type. Otherwise, just + * transform the argument normally. + */ + if (IsA(tc->arg, A_ArrayExpr)) + { + Oid targetBaseType; + int32 targetBaseTypmod; + Oid elementType; + + /* + * If target is a domain over array, work with the base array type + * here. Below, we'll cast the array type to the domain. In the + * usual case that the target is not a domain, the remaining steps + * will be a no-op. + */ + targetBaseTypmod = targetTypmod; + targetBaseType = getBaseTypeAndTypmod(targetType, &targetBaseTypmod); + elementType = get_element_type(targetBaseType); + if (OidIsValid(elementType)) + { + expr = transformArrayExpr(pstate, + (A_ArrayExpr *) tc->arg, + targetBaseType, + elementType, + targetBaseTypmod); + } + else + expr = transformExprRecurse(pstate, tc->arg); + } + else + expr = transformExprRecurse(pstate, tc->arg); + + inputType = exprType(expr); if (inputType == InvalidOid) return expr; /* do nothing if NULL input */ diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 7540043ce5..c29f106529 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -1072,7 +1072,9 @@ generateClonedIndexStmt(CreateStmtContext *cxt, Relation source_idx, index->oldNode = InvalidOid; index->unique = idxrec->indisunique; index->primary = idxrec->indisprimary; + index->transformed = true; /* don't need transformIndexStmt */ index->concurrent = false; + index->if_not_exists = false; /* * We don't try to preserve the name of the source index; instead, just @@ -1530,7 +1532,9 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) index->idxcomment = NULL; index->indexOid = InvalidOid; index->oldNode = InvalidOid; + index->transformed = false; index->concurrent = false; + index->if_not_exists = false; /* * If it's ALTER TABLE ADD CONSTRAINT USING INDEX, look up the index and @@ -1941,6 +1945,10 @@ transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString) ListCell *l; Relation rel; + /* Nothing to do if statement already transformed. */ + if (stmt->transformed) + return stmt; + /* * We must not scribble on the passed-in IndexStmt, so copy it. (This is * overkill, but easy.) @@ -2021,6 +2029,9 @@ transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString) /* Close relation */ heap_close(rel, NoLock); + /* Mark statement as successfully transformed */ + stmt->transformed = true; + return stmt; } diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 35b68ec033..d7b6148cd5 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2261,6 +2261,7 @@ typedef struct IndexStmt bool isconstraint; /* is it for a pkey/unique constraint? */ bool deferrable; /* is the constraint DEFERRABLE? */ bool initdeferred; /* is the constraint INITIALLY DEFERRED? */ + bool transformed; /* true when transformIndexStmt is finished */ bool concurrent; /* should this be a concurrent index build? */ bool if_not_exists; /* just do nothing if index already exists? */ } IndexStmt; -- 2.40.0