Get rid of multiple applications of transformExpr() to the same tree.
authorTom Lane <tgl@sss.pgh.pa.us>
Sun, 22 Feb 2015 18:59:09 +0000 (13:59 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Sun, 22 Feb 2015 18:59:09 +0000 (13:59 -0500)
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
src/backend/commands/tablecmds.c
src/backend/nodes/copyfuncs.c
src/backend/nodes/equalfuncs.c
src/backend/nodes/outfuncs.c
src/backend/parser/gram.y
src/backend/parser/parse_clause.c
src/backend/parser/parse_expr.c
src/backend/parser/parse_utilcmd.c
src/include/nodes/parsenodes.h

index 9edd1a0aff908c1bbff3d3912e0d777015f0452e..fdb1f7faacec9bcd4aaa7b7235281c370f8246ee 100644 (file)
@@ -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,
index b2993b80fbc255aecf0ff4039ff89f9b65e4a7e4..f5d5b6302f6dbf62dd01a9b5ca32fc519665275c 100644 (file)
@@ -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 */
index e5b0dce477d2489bac096cc007202f8be1891539..8798cbe43db5abeb50313cce0de79a1111e3281f 100644 (file)
@@ -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);
 
index 6e8b308a3e076306784950bd87cecee4dc35b344..a90386b8004c91462546898f1bfc4225b44321c8 100644 (file)
@@ -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);
 
index d8e2077d6050f76880ca209d994003b091478479..67b7d40406a90ae34babe2cda38ebe7eefd0fdc7 100644 (file)
@@ -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
index 67c7c84deecb4ab05a4f7facb91df32f1ff39c17..b8af29639790bbb4e1398023af3de2af63c158ab 100644 (file)
@@ -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;
                                }
index 654dce6755e7e82fd4948809d39c305aa2775c1f..8d90b5098a13f05a81df53bcea79d26d6e6f1697 100644 (file)
@@ -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)
        {
index 67a2310ad0e0e881f94a66cff90f34420700980a..791639a7db351f7fa170c55606b388d8328da47f 100644 (file)
@@ -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 */
 
index 7540043ce50625fddad2767c5a7f5ea0e4563121..c29f1065294a42c4b3d38fcc4c0bce6d3baa4681 100644 (file)
@@ -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;
 }
 
index 35b68ec033252e8a76666fae68efd48fdbd9dda5..d7b6148cd5e26e30656678d87cf1215666141914 100644 (file)
@@ -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;