Get rid of multiple applications of transformExpr() to the same tree.

tglsfdc · tglsfdc · commit 6a75562ed16b · 2015-02-22T13:59:09.000-05:00
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.
diff --git 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
@@ -5705,15 +5705,16 @@ 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 */
 	skip_build = tab->rewrite > 0 || OidIsValid(stmt->oldNode);
 	/* 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;

Original file line number	Diff line number	Diff line change
`@@ -2078,7 +2078,9 @@ _outIndexStmt(StringInfo str, const IndexStmt *node)`
`2078`	`2078`	`WRITE_BOOL_FIELD(isconstraint);`
`2079`	`2079`	`WRITE_BOOL_FIELD(deferrable);`
`2080`	`2080`	`WRITE_BOOL_FIELD(initdeferred);`
	`2081`	`+ WRITE_BOOL_FIELD(transformed);`
`2081`	`2082`	`WRITE_BOOL_FIELD(concurrent);`
	`2083`	`+ WRITE_BOOL_FIELD(if_not_exists);`
`2082`	`2084`	`}`
`2083`	`2085`
`2084`	`2086`	`static void`