]> granicus.if.org Git - postgresql/commitdiff
Fix extremely nasty little bug observed when a sub-SELECT appears in
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 4 Apr 2000 01:21:48 +0000 (01:21 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 4 Apr 2000 01:21:48 +0000 (01:21 +0000)
WHERE in a place where it can be part of a nestloop inner indexqual.
As the code stood, it put the same physical sub-Plan node into both
indxqual and indxqualorig of the IndexScan plan node.  That confused
later processing in the optimizer (which expected that tracing the
subPlan list would visit each subplan node exactly once), and would
probably have blown up in the executor if the planner hadn't choked first.
Fix by making the 'fixed' indexqual be a complete deep copy of the
original indexqual, rather than trying to share nodes below the topmost
operator node.  This had further ramifications though, because we were
making the aforesaid list of sub-Plan nodes during SS_process_sublinks
which is run before construction of the 'fixed' indexqual, meaning that
the copy of the sub-Plan didn't show up in that list.  Fix by rearranging
logic so that the sub-Plan list is built by the final set_plan_references
pass, not in SS_process_sublinks.  This may sound like a mess, but it's
actually a good deal cleaner now than it was before, because we are no
longer dependent on the assumption that planning will never make a copy
of a sub-Plan node.

src/backend/nodes/copyfuncs.c
src/backend/optimizer/plan/createplan.c
src/backend/optimizer/plan/setrefs.c
src/backend/optimizer/plan/subselect.c
src/backend/optimizer/util/clauses.c
src/include/optimizer/clauses.h
src/include/optimizer/subselect.h

index 836687240d5cf0cb39058affaf3fa50e2de81ab3..41918cda14200df4ee55f739e760223bb4fd54ce 100644 (file)
@@ -8,15 +8,15 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v 1.110 2000/03/22 22:08:32 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v 1.111 2000/04/04 01:21:48 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 
 #include "postgres.h"
 
+#include "optimizer/clauses.h"
 #include "optimizer/planmain.h"
-#include "optimizer/subselect.h"
 
 
 /*
@@ -88,9 +88,10 @@ CopyPlanFields(Plan *from, Plan *newnode)
        newnode->locParam = listCopy(from->locParam);
        newnode->chgParam = listCopy(from->chgParam);
        Node_Copy(from, newnode, initPlan);
+       /* subPlan list must point to subplans in the new subtree, not the old */
        if (from->subPlan != NIL)
-               newnode->subPlan = nconc(SS_pull_subplan((Node *) newnode->targetlist),
-                                                                SS_pull_subplan((Node *) newnode->qual));
+               newnode->subPlan = nconc(pull_subplans((Node *) newnode->targetlist),
+                                                                pull_subplans((Node *) newnode->qual));
        else
                newnode->subPlan = NIL;
        newnode->nParamExec = from->nParamExec;
@@ -142,7 +143,7 @@ _copyResult(Result *from)
         */
        if (from->plan.subPlan != NIL)
                newnode->plan.subPlan = nconc(newnode->plan.subPlan,
-                                                                         SS_pull_subplan(newnode->resconstantqual));
+                                                                         pull_subplans(newnode->resconstantqual));
 
        return newnode;
 }
@@ -252,6 +253,17 @@ _copyIndexScan(IndexScan *from)
        Node_Copy(from, newnode, indxqualorig);
        newnode->indxorderdir = from->indxorderdir;
 
+       /*
+        * We must add subplans in index quals to the new plan's subPlan list
+        */
+       if (from->scan.plan.subPlan != NIL)
+       {
+               newnode->scan.plan.subPlan = nconc(newnode->scan.plan.subPlan,
+                                                                                  pull_subplans((Node *) newnode->indxqual));
+               newnode->scan.plan.subPlan = nconc(newnode->scan.plan.subPlan,
+                                                                                  pull_subplans((Node *) newnode->indxqualorig));
+       }
+
        return newnode;
 }
 
@@ -358,6 +370,13 @@ _copyMergeJoin(MergeJoin *from)
         */
        Node_Copy(from, newnode, mergeclauses);
 
+       /*
+        * We must add subplans in mergeclauses to the new plan's subPlan list
+        */
+       if (from->join.subPlan != NIL)
+               newnode->join.subPlan = nconc(newnode->join.subPlan,
+                                                                         pull_subplans((Node *) newnode->mergeclauses));
+
        return newnode;
 }
 
@@ -382,9 +401,15 @@ _copyHashJoin(HashJoin *from)
         * ----------------
         */
        Node_Copy(from, newnode, hashclauses);
-
        newnode->hashjoinop = from->hashjoinop;
 
+       /*
+        * We must add subplans in hashclauses to the new plan's subPlan list
+        */
+       if (from->join.subPlan != NIL)
+               newnode->join.subPlan = nconc(newnode->join.subPlan,
+                                                                         pull_subplans((Node *) newnode->hashclauses));
+
        return newnode;
 }
 
index 8e20ae302e1971bb567cb0c07071af8c0cc7cd82..3fab7f08b877d8922b5bde4df5456104407cd093 100644 (file)
@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/createplan.c,v 1.87 2000/03/22 22:08:34 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/createplan.c,v 1.88 2000/04/04 01:21:47 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -724,7 +724,9 @@ create_hashjoin_node(HashPath *best_path,
  * track of which index applies to each subgroup of index qual clauses...
  *
  * Returns a modified copy of the indexqual list --- the original is not
- * changed.
+ * changed.  Note also that the copy shares no substructure with the
+ * original; this is needed in case there is a subplan in it (we need
+ * two separate copies of the subplan tree, or things will go awry).
  */
 
 static List *
@@ -808,11 +810,13 @@ fix_indxqual_sublist(List *indexqual, int baserelid, Oid relam,
                get_relattval((Node *) clause, baserelid,
                                          &relid, &attno, &constval, &flag);
 
-               /* Copy enough structure to allow commuting and replacing an operand
-                * without changing original clause.
+               /* Make a copy that will become the fixed clause.
+                *
+                * We used to try to do a shallow copy here, but that fails if there
+                * is a subplan in the arguments of the opclause.  So just do a
+                * full copy.
                 */
-               newclause = make_clause(clause->opType, clause->oper,
-                                                               listCopy(clause->args));
+               newclause = (Expr *) copyObject((Node *) clause);
 
                /* If the indexkey is on the right, commute the clause. */
                if ((flag & SEL_RIGHT) == 0)
@@ -834,11 +838,7 @@ fix_indxqual_sublist(List *indexqual, int baserelid, Oid relam,
                newopno = indexable_operator(newclause, opclass, relam, true);
                if (newopno == InvalidOid)
                        elog(ERROR, "fix_indxqual_sublist: failed to find substitute op");
-               if (newopno != ((Oper *) newclause->oper)->opno)
-               {
-                       newclause->oper = (Node *) copyObject(newclause->oper);
-                       ((Oper *) newclause->oper)->opno = newopno;
-               }
+               ((Oper *) newclause->oper)->opno = newopno;
 
                fixed_qual = lappend(fixed_qual, newclause);
        }
index b44aa6408b808692469e6e47aadde2723c271096..756333e005957ce4d1c96822a17766a8a956c448 100644 (file)
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/setrefs.c,v 1.60 2000/01/26 05:56:38 momjian Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/setrefs.c,v 1.61 2000/04/04 01:21:47 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -59,6 +59,8 @@ static bool fix_opids_walker(Node *node, void *context);
  *       for the convenience of the executor.  We update Vars in upper plan nodes
  *       to refer to the outputs of their subplans, and we compute regproc OIDs
  *       for operators (ie, we look up the function that implements each op).
+ *       We must also build lists of all the subplan nodes present in each
+ *       plan node's expression trees.
  *
  *       set_plan_references recursively traverses the whole plan tree.
  *
@@ -72,6 +74,11 @@ set_plan_references(Plan *plan)
        if (plan == NULL)
                return;
 
+       /* We must rebuild the plan's list of subplan nodes, since we are
+        * copying/mutating its expression trees.
+        */
+       plan->subPlan = NIL;
+
        /*
         * Plan-type-specific fixes
         */
@@ -83,6 +90,12 @@ set_plan_references(Plan *plan)
                case T_IndexScan:
                        fix_opids((Node *) ((IndexScan *) plan)->indxqual);
                        fix_opids((Node *) ((IndexScan *) plan)->indxqualorig);
+                       plan->subPlan =
+                               nconc(plan->subPlan,
+                                         pull_subplans((Node *) ((IndexScan *) plan)->indxqual));
+                       plan->subPlan =
+                               nconc(plan->subPlan,
+                                         pull_subplans((Node *) ((IndexScan *) plan)->indxqualorig));
                        break;
                case T_NestLoop:
                        set_join_references((Join *) plan);
@@ -90,10 +103,16 @@ set_plan_references(Plan *plan)
                case T_MergeJoin:
                        set_join_references((Join *) plan);
                        fix_opids((Node *) ((MergeJoin *) plan)->mergeclauses);
+                       plan->subPlan =
+                               nconc(plan->subPlan,
+                                         pull_subplans((Node *) ((MergeJoin *) plan)->mergeclauses));
                        break;
                case T_HashJoin:
                        set_join_references((Join *) plan);
                        fix_opids((Node *) ((HashJoin *) plan)->hashclauses);
+                       plan->subPlan =
+                               nconc(plan->subPlan,
+                                         pull_subplans((Node *) ((HashJoin *) plan)->hashclauses));
                        break;
                case T_Material:
                case T_Sort:
@@ -119,6 +138,9 @@ set_plan_references(Plan *plan)
                        if (plan->lefttree != NULL)
                                set_uppernode_references(plan, (Index) OUTER);
                        fix_opids(((Result *) plan)->resconstantqual);
+                       plan->subPlan =
+                               nconc(plan->subPlan,
+                                         pull_subplans(((Result *) plan)->resconstantqual));
                        break;
                case T_Append:
                        foreach(pl, ((Append *) plan)->appendplans)
@@ -136,10 +158,17 @@ set_plan_references(Plan *plan)
        }
 
        /*
-        * For all plan types, fix operators in targetlist and qual expressions
+        * For all plan types, fix operators in targetlist and qual expressions,
+        * and find subplans therein.
         */
        fix_opids((Node *) plan->targetlist);
        fix_opids((Node *) plan->qual);
+       plan->subPlan =
+               nconc(plan->subPlan,
+                         pull_subplans((Node *) plan->targetlist));
+       plan->subPlan =
+               nconc(plan->subPlan,
+                         pull_subplans((Node *) plan->qual));
 
        /*
         * Now recurse into subplans, if any
@@ -302,31 +331,7 @@ join_references_mutator(Node *node,
                 */
                if (var->varno != context->acceptable_rel)
                        elog(ERROR, "join_references: variable not in subplan target lists");
-               return (Node *) newvar; /* copy is probably not necessary here... */
-       }
-       /*
-        * expression_tree_mutator will copy SubPlan nodes if given a chance.
-        * We do not want to do that here, because subselect.c has already
-        * constructed the initPlan and subPlan lists of the current plan node
-        * and we mustn't leave those dangling (ie, pointing to different
-        * copies of the nodes than what's in the targetlist & quals...)
-        * Instead, alter the SubPlan in-place.  Grotty --- is there a better way?
-        */
-       if (is_subplan(node))
-       {
-               Expr       *expr = (Expr *) node;
-               SubLink    *sublink = ((SubPlan *) expr->oper)->sublink;
-
-               /* transform args list (params to be passed to subplan) */
-               expr->args = (List *)
-                       join_references_mutator((Node *) expr->args,
-                                                                       context);
-               /* transform sublink's oper list as well */
-               sublink->oper = (List *)
-                       join_references_mutator((Node *) sublink->oper,
-                                                                       context);
-
-               return (Node *) expr;
+               return (Node *) newvar;
        }
        return expression_tree_mutator(node,
                                                                   join_references_mutator,
@@ -383,30 +388,6 @@ replace_vars_with_subplan_refs_mutator(Node *node,
                newvar->varattno = resdom->resno;
                return (Node *) newvar;
        }
-       /*
-        * expression_tree_mutator will copy SubPlan nodes if given a chance.
-        * We do not want to do that here, because subselect.c has already
-        * constructed the initPlan and subPlan lists of the current plan node
-        * and we mustn't leave those dangling (ie, pointing to different
-        * copies of the nodes than what's in the targetlist & quals...)
-        * Instead, alter the SubPlan in-place.  Grotty --- is there a better way?
-        */
-       if (is_subplan(node))
-       {
-               Expr       *expr = (Expr *) node;
-               SubLink    *sublink = ((SubPlan *) expr->oper)->sublink;
-
-               /* transform args list (params to be passed to subplan) */
-               expr->args = (List *)
-                       replace_vars_with_subplan_refs_mutator((Node *) expr->args,
-                                                                                                  context);
-               /* transform sublink's oper list as well */
-               sublink->oper = (List *)
-                       replace_vars_with_subplan_refs_mutator((Node *) sublink->oper,
-                                                                                                  context);
-
-               return (Node *) expr;
-       }
        return expression_tree_mutator(node,
                                                                   replace_vars_with_subplan_refs_mutator,
                                                                   (void *) context);
index 3928f5783604a784e6aaf742c6d4b7df6661545d..b77d8b586f60a95bfe31946a9597559b27ffadf3 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/subselect.c,v 1.33 2000/03/21 05:12:02 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/subselect.c,v 1.34 2000/04/04 01:21:47 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -442,6 +442,12 @@ set_unioni(List *l1, List *l2)
  * finalize_primnode: build lists of subplans and params appearing
  * in the given expression tree.  NOTE: items are added to lists passed in,
  * so caller must initialize lists to NIL before first call!
+ *
+ * Note: the subplan list that is constructed here and assigned to the
+ * plan's subPlan field will be replaced with an up-to-date list in
+ * set_plan_references().  We could almost dispense with building this
+ * subplan list at all; I believe the only place that uses it is the
+ * check in make_subplan to see whether a subselect has any subselects.
  */
 
 typedef struct finalize_primnode_results {
@@ -604,6 +610,10 @@ SS_finalize_plan(Plan *plan)
                case T_IndexScan:
                        finalize_primnode((Node *) ((IndexScan *) plan)->indxqual,
                                                          &results);
+                       /* we need not look at indxqualorig, since it will have the
+                        * same param references as indxqual, and we aren't really
+                        * concerned yet about having a complete subplan list.
+                        */
                        break;
 
                case T_MergeJoin:
@@ -670,32 +680,3 @@ SS_finalize_plan(Plan *plan)
 
        return results.paramids;
 }
-
-/*
- * Construct a list of all subplans found within the given node tree.
- */
-
-static bool SS_pull_subplan_walker(Node *node, List **listptr);
-
-List *
-SS_pull_subplan(Node *expr)
-{
-       List       *result = NIL;
-
-       SS_pull_subplan_walker(expr, &result);
-       return result;
-}
-
-static bool
-SS_pull_subplan_walker(Node *node, List **listptr)
-{
-       if (node == NULL)
-               return false;
-       if (is_subplan(node))
-       {
-               *listptr = lappend(*listptr, ((Expr *) node)->oper);
-               /* fall through to check args to subplan */
-       }
-       return expression_tree_walker(node, SS_pull_subplan_walker,
-                                                                 (void *) listptr);
-}
index 8d4dae486ac7f7793a23d69120f839120b0d60ad..d429db93a03cdb384cc5b334d656d12f5ed4b8a7 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.63 2000/03/21 05:12:12 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.64 2000/04/04 01:21:46 tgl Exp $
  *
  * HISTORY
  *       AUTHOR                        DATE                    MAJOR EVENT
@@ -43,6 +43,8 @@
 
 static bool contain_agg_clause_walker(Node *node, void *context);
 static bool pull_agg_clause_walker(Node *node, List **listptr);
+static bool contain_subplans_walker(Node *node, void *context);
+static bool pull_subplans_walker(Node *node, List **listptr);
 static bool check_subplans_for_ungrouped_vars_walker(Node *node,
                                                                                                         Query *context);
 static int is_single_func(Node *node);
@@ -358,39 +360,9 @@ make_ands_implicit(Expr *clause)
 
 
 /*****************************************************************************
- *                                                                                                                                                      *
- *             General clause-manipulating routines                                                             *
- *                                                                                                                                                      *
+ *             Aggregate-function clause manipulation
  *****************************************************************************/
 
-
-/*
- * pull_constant_clauses
- *       Scans through a list of qualifications and find those that
- *       contain no variables (of the current query level).
- *
- * Returns a list of the constant clauses in constantQual and the remaining
- * quals as the return value.
- *
- */
-List *
-pull_constant_clauses(List *quals, List **constantQual)
-{
-       List       *q;
-       List       *constqual = NIL;
-       List       *restqual = NIL;
-
-       foreach(q, quals)
-       {
-               if (!contain_var_clause(lfirst(q)))
-                       constqual = lcons(lfirst(q), constqual);
-               else
-                       restqual = lcons(lfirst(q), restqual);
-       }
-       *constantQual = constqual;
-       return restqual;
-}
-
 /*
  * contain_agg_clause
  *       Recursively search for Aggref nodes within a clause.
@@ -454,6 +426,68 @@ pull_agg_clause_walker(Node *node, List **listptr)
                                                                  (void *) listptr);
 }
 
+
+/*****************************************************************************
+ *             Subplan clause manipulation
+ *****************************************************************************/
+
+/*
+ * contain_subplans
+ *       Recursively search for subplan nodes within a clause.
+ *
+ * If we see a SubLink node, we will return TRUE.  This is only possible if
+ * the expression tree hasn't yet been transformed by subselect.c.  We do not
+ * know whether the node will produce a true subplan or just an initplan,
+ * but we make the conservative assumption that it will be a subplan.
+ *
+ * Returns true if any subplan found.
+ */
+bool
+contain_subplans(Node *clause)
+{
+       return contain_subplans_walker(clause, NULL);
+}
+
+static bool
+contain_subplans_walker(Node *node, void *context)
+{
+       if (node == NULL)
+               return false;
+       if (is_subplan(node) || IsA(node, SubLink))
+               return true;                    /* abort the tree traversal and return true */
+       return expression_tree_walker(node, contain_subplans_walker, context);
+}
+
+/*
+ * pull_subplans
+ *       Recursively pulls all subplans from an expression tree.
+ *
+ *       Returns list of subplan nodes found.  Note the nodes themselves are not
+ *       copied, only referenced.
+ */
+List *
+pull_subplans(Node *clause)
+{
+       List       *result = NIL;
+
+       pull_subplans_walker(clause, &result);
+       return result;
+}
+
+static bool
+pull_subplans_walker(Node *node, List **listptr)
+{
+       if (node == NULL)
+               return false;
+       if (is_subplan(node))
+       {
+               *listptr = lappend(*listptr, ((Expr *) node)->oper);
+               /* fall through to check args to subplan */
+       }
+       return expression_tree_walker(node, pull_subplans_walker,
+                                                                 (void *) listptr);
+}
+
 /*
  * check_subplans_for_ungrouped_vars
  *             Check for subplans that are being passed ungrouped variables as
@@ -556,6 +590,41 @@ check_subplans_for_ungrouped_vars_walker(Node *node,
 }
 
 
+/*****************************************************************************
+ *                                                                                                                                                      *
+ *             General clause-manipulating routines                                                             *
+ *                                                                                                                                                      *
+ *****************************************************************************/
+
+
+/*
+ * pull_constant_clauses
+ *       Scans through a list of qualifications and find those that
+ *       contain no variables (of the current query level).
+ *
+ * Returns a list of the constant clauses in constantQual and the remaining
+ * quals as the return value.
+ *
+ */
+List *
+pull_constant_clauses(List *quals, List **constantQual)
+{
+       List       *q;
+       List       *constqual = NIL;
+       List       *restqual = NIL;
+
+       foreach(q, quals)
+       {
+               if (!contain_var_clause(lfirst(q)))
+                       constqual = lcons(lfirst(q), constqual);
+               else
+                       restqual = lcons(lfirst(q), restqual);
+       }
+       *constantQual = constqual;
+       return restqual;
+}
+
+
 /*
  * clause_relids_vars
  *       Retrieves distinct relids and vars appearing within a clause.
@@ -726,7 +795,8 @@ default_results:
  *   If the given expression is a function of a single relation,
  *   return the relation number; else return 0
  */
-static int is_single_func(Node *node)
+static int
+is_single_func(Node *node)
 {
        if (is_funcclause(node))
        {
index ef2bfeb63b63c550b53a5a8e654d255cf7e014ab..a6f6ea2e5f3d0e0f8bd2183e4c2c44978d4c4b2d 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2000, PostgreSQL, Inc
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: clauses.h,v 1.34 2000/03/21 05:11:51 tgl Exp $
+ * $Id: clauses.h,v 1.35 2000/04/04 01:21:45 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -38,11 +38,15 @@ extern Expr *make_andclause(List *andclauses);
 extern Expr *make_ands_explicit(List *andclauses);
 extern List *make_ands_implicit(Expr *clause);
 
-extern List *pull_constant_clauses(List *quals, List **constantQual);
 extern bool contain_agg_clause(Node *clause);
 extern List *pull_agg_clause(Node *clause);
+
+extern bool contain_subplans(Node *clause);
+extern List *pull_subplans(Node *clause);
 extern void check_subplans_for_ungrouped_vars(Node *clause, Query *query);
 
+extern List *pull_constant_clauses(List *quals, List **constantQual);
+
 extern void clause_get_relids_vars(Node *clause, Relids *relids, List **vars);
 extern int     NumRelids(Node *clause);
 extern void get_relattval(Node *clause, int targetrelid,
index a00158deb9461e5556d1f0fabfe8e0464c87c527..f4e511216133648459e8c7ecb5b59ff60d6c1edb 100644 (file)
@@ -17,6 +17,5 @@ extern int    PlannerPlanId;          /* to assign unique ID to subquery plans */
 extern List *SS_finalize_plan(Plan *plan);
 extern Node *SS_replace_correlation_vars(Node *expr);
 extern Node *SS_process_sublinks(Node *expr);
-extern List *SS_pull_subplan(Node *expr);
 
 #endif  /* SUBSELECT_H */