From b4210ae0f03cb680176286910845fb0d6e99c8a1 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 6 Jun 1999 17:38:11 +0000 Subject: [PATCH] Fix problems with grouping/aggregation in queries that use inheritance ... basically it was completely busted :-( --- src/backend/optimizer/plan/planner.c | 32 +++++++-- src/backend/optimizer/plan/setrefs.c | 91 +------------------------- src/backend/optimizer/prep/prepunion.c | 81 ++++++++++++++--------- src/include/optimizer/planmain.h | 3 +- src/include/optimizer/prep.h | 11 ++-- 5 files changed, 87 insertions(+), 131 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 65327a1263..6dc76f0fa2 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -7,7 +7,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v 1.54 1999/05/25 16:09:37 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v 1.55 1999/06/06 17:38:10 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -122,14 +122,36 @@ union_planner(Query *parse) } else if ((rt_index = first_inherit_rt_entry(rangetable)) != -1) { - if (parse->rowMark != NULL) - elog(ERROR, "SELECT FOR UPDATE is not supported for inherit queries"); - result_plan = (Plan *) plan_inherit_queries(parse, rt_index); - /* XXX do we need to do this? bjm 12/19/97 */ + List *sub_tlist; + + /* + * Generate appropriate target list for subplan; may be different + * from tlist if grouping or aggregation is needed. + */ + sub_tlist = make_subplanTargetList(parse, tlist, &groupColIdx); + + /* + * Recursively plan the subqueries needed for inheritance + */ + result_plan = (Plan *) plan_inherit_queries(parse, sub_tlist, + rt_index); + + /* + * Fix up outer target list. NOTE: unlike the case for non-inherited + * query, we pass the unfixed tlist to subplans, which do their own + * fixing. But we still want to fix the outer target list afterwards. + * I *think* this is correct --- doing the fix before recursing is + * definitely wrong, because preprocess_targetlist() will do the + * wrong thing if invoked twice on the same list. Maybe that is a bug? + * tgl 6/6/99 + */ tlist = preprocess_targetlist(tlist, parse->commandType, parse->resultRelation, parse->rtable); + + if (parse->rowMark != NULL) + elog(ERROR, "SELECT FOR UPDATE is not supported for inherit queries"); } else { diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 4cb945b5f6..0ba84d3ba9 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -7,7 +7,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/optimizer/plan/setrefs.c,v 1.49 1999/05/26 12:55:28 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/optimizer/plan/setrefs.c,v 1.50 1999/06/06 17:38:10 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -45,7 +45,6 @@ static Var *replace_joinvar_refs(Var *var, static List *tlist_noname_references(Oid nonameid, List *tlist); static bool OperandIsInner(Node *opnd, int inner_relid); static List *pull_agg_clause(Node *clause); -static Node *del_agg_clause(Node *clause); static void set_result_tlist_references(Result *resultNode); static void replace_vars_with_subplan_refs(Node *clause, Index subvarno, @@ -874,94 +873,6 @@ pull_agg_clause(Node *clause) } -/* - * del_agg_tlist_references - * Remove the Agg nodes from the target list - * We do this so inheritance only does aggregates in the upper node - */ -void -del_agg_tlist_references(List *tlist) -{ - List *tl; - - foreach(tl, tlist) - { - TargetEntry *tle = lfirst(tl); - - tle->expr = del_agg_clause(tle->expr); - } -} - -static Node * -del_agg_clause(Node *clause) -{ - List *t; - - if (clause == NULL) - return clause; - - if (IsA(clause, Var)) - return clause; - else if (is_funcclause(clause)) - { - - /* - * This is a function. Recursively call this routine for its - * arguments... - */ - foreach(t, ((Expr *) clause)->args) - lfirst(t) = del_agg_clause(lfirst(t)); - } - else if (IsA(clause, Aggref)) - { - - /* here is the real action, to remove the Agg node */ - return del_agg_clause(((Aggref *) clause)->target); - - } - else if (IsA(clause, ArrayRef)) - { - ArrayRef *aref = (ArrayRef *) clause; - - /* - * This is an arrayref. Recursively call this routine for its - * expression and its index expression... - */ - foreach(t, aref->refupperindexpr) - lfirst(t) = del_agg_clause(lfirst(t)); - foreach(t, aref->reflowerindexpr) - lfirst(t) = del_agg_clause(lfirst(t)); - aref->refexpr = del_agg_clause(aref->refexpr); - aref->refassgnexpr = del_agg_clause(aref->refassgnexpr); - } - else if (is_opclause(clause)) - { - - /* - * This is an operator. Recursively call this routine for both its - * left and right operands - */ - Node *left = (Node *) get_leftop((Expr *) clause); - Node *right = (Node *) get_rightop((Expr *) clause); - - if (left != (Node *) NULL) - left = del_agg_clause(left); - if (right != (Node *) NULL) - right = del_agg_clause(right); - } - else if (IsA(clause, Param) ||IsA(clause, Const)) - return clause; - else - { - - /* - * Ooops! we can not handle that! - */ - elog(ERROR, "del_agg_clause: Can not handle this tlist!\n"); - } - return NULL; -} - /* * check_having_for_ungrouped_vars takes the havingQual and the list of * GROUP BY clauses and checks for subplans in the havingQual that are being diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index 6bd493a488..478186b631 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -7,7 +7,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/optimizer/prep/prepunion.c,v 1.31 1999/05/25 16:09:47 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/optimizer/prep/prepunion.c,v 1.32 1999/06/06 17:38:11 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -35,7 +35,7 @@ #include "optimizer/planmain.h" static List *plan_inherit_query(Relids relids, Index rt_index, - RangeTblEntry *rt_entry, Query *parse, + RangeTblEntry *rt_entry, Query *parse, List *tlist, List **union_rtentriesPtr); static RangeTblEntry *new_rangetable_entry(Oid new_relid, RangeTblEntry *old_entry); @@ -208,19 +208,33 @@ plan_union_queries(Query *parse) * * Plans the queries for a given parent relation. * - * Returns a list containing a list of plans and a list of rangetable - * entries to be inserted into an APPEND node. - * XXX - what exactly does this mean, look for make_append + * Inputs: + * parse = parent parse tree + * tlist = target list for inheritance subqueries (not same as parent's!) + * rt_index = rangetable index for current inheritance item + * + * Returns an APPEND node that forms the result of performing the given + * query for each member relation of the inheritance group. + * + * If grouping, aggregation, or sorting is specified in the parent plan, + * the subplans should not do any of those steps --- we must do those + * operations just once above the APPEND node. The given tlist has been + * modified appropriately to remove group/aggregate expressions, but the + * Query node still has the relevant fields set. We remove them in the + * copies used for subplans (see plan_inherit_query). + * + * NOTE: this can be invoked recursively if more than one inheritance wildcard + * is present. At each level of recursion, the first wildcard remaining in + * the rangetable is expanded. */ Append * -plan_inherit_queries(Query *parse, Index rt_index) +plan_inherit_queries(Query *parse, List *tlist, Index rt_index) { - List *union_plans = NIL; - List *rangetable = parse->rtable; RangeTblEntry *rt_entry = rt_fetch(rt_index, rangetable); List *inheritrtable = NIL; - List *union_relids = NIL; + List *union_relids; + List *union_plans; union_relids = find_all_inheritors(lconsi(rt_entry->relid, NIL), @@ -228,12 +242,12 @@ plan_inherit_queries(Query *parse, Index rt_index) /* * Remove the flag for this relation, since we're about to handle it - * (do it before recursing!). XXX destructive parse tree change + * (do it before recursing!). XXX destructive change to parent parse tree */ - rt_fetch(rt_index, rangetable)->inh = false; + rt_entry->inh = false; union_plans = plan_inherit_query(union_relids, rt_index, rt_entry, - parse, &inheritrtable); + parse, tlist, &inheritrtable); return (make_append(union_plans, NULL, @@ -252,11 +266,12 @@ plan_inherit_query(Relids relids, Index rt_index, RangeTblEntry *rt_entry, Query *root, + List *tlist, List **union_rtentriesPtr) { - List *i; List *union_plans = NIL; List *union_rtentries = NIL; + List *i; foreach(i, relids) { @@ -268,19 +283,21 @@ plan_inherit_query(Relids relids, new_rt_entry); /* - * reset the uniqueflag and sortclause in parse tree root, so that - * sorting will only be done once after append + * Insert the desired simplified tlist into the subquery + */ + new_root->targetList = copyObject(tlist); + + /* + * Clear the sorting and grouping qualifications in the subquery, + * so that sorting will only be done once after append */ new_root->uniqueFlag = NULL; new_root->sortClause = NULL; new_root->groupClause = NULL; new_root->havingQual = NULL; + new_root->hasAggs = false; /* shouldn't be any left ... */ - if (new_root->hasAggs) - { - new_root->hasAggs = false; - del_agg_tlist_references(new_root->targetList); - } + /* Fix attribute numbers as necessary */ fix_parsetree_attnums(rt_index, rt_entry->relid, relid, @@ -346,7 +363,7 @@ int first_inherit_rt_entry(List *rangetable) { int count = 0; - List *temp = NIL; + List *temp; foreach(temp, rangetable) { @@ -393,8 +410,8 @@ static Query * subst_rangetable(Query *root, Index index, RangeTblEntry *new_entry) { Query *new_root = copyObject(root); - List *temp = NIL; - int i = 0; + List *temp; + int i; for (temp = new_root->rtable, i = 1; i < index; temp = lnext(temp), i++) ; @@ -403,6 +420,15 @@ subst_rangetable(Query *root, Index index, RangeTblEntry *new_entry) return new_root; } +/* + * Adjust varnos for child tables. This routine makes it possible for + * child tables to have different column positions for the "same" attribute + * as a parent, which helps ALTER TABLE ADD COLUMN. Unfortunately this isn't + * nearly enough to make it work transparently; there are other places where + * things fall down if children and parents don't have the same column numbers + * for inherited attributes. It'd be better to rip this code out and fix + * ALTER TABLE... + */ static void fix_parsetree_attnums_nodes(Index rt_index, Oid old_relid, @@ -433,16 +459,11 @@ fix_parsetree_attnums_nodes(Index rt_index, case T_Var: { Var *var = (Var *) node; - Oid old_typeid, - new_typeid; - - old_typeid = old_relid; - new_typeid = new_relid; if (var->varno == rt_index && var->varattno != 0) { - var->varattno = get_attnum(new_typeid, - get_attname(old_typeid, var->varattno)); + var->varattno = get_attnum(new_relid, + get_attname(old_relid, var->varattno)); } } break; diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h index edf4da525e..96bf3ebfc1 100644 --- a/src/include/optimizer/planmain.h +++ b/src/include/optimizer/planmain.h @@ -6,7 +6,7 @@ * * Copyright (c) 1994, Regents of the University of California * - * $Id: planmain.h,v 1.27 1999/05/26 12:56:36 momjian Exp $ + * $Id: planmain.h,v 1.28 1999/06/06 17:38:09 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -58,7 +58,6 @@ extern void replace_tlist_with_subplan_refs(List *tlist, Index subvarno, List *subplanTargetList); extern bool set_agg_tlist_references(Agg *aggNode); -extern void del_agg_tlist_references(List *tlist); extern void check_having_for_ungrouped_vars(Node *clause, List *groupClause, List *targetList); diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h index 24e83d340d..c3b06101c6 100644 --- a/src/include/optimizer/prep.h +++ b/src/include/optimizer/prep.h @@ -6,7 +6,7 @@ * * Copyright (c) 1994, Regents of the University of California * - * $Id: prep.h,v 1.14 1999/02/13 23:21:52 momjian Exp $ + * $Id: prep.h,v 1.15 1999/06/06 17:38:10 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -17,20 +17,23 @@ #include /* - * prototypes for prepqual.h + * prototypes for prepqual.c */ extern List *cnfify(Expr *qual, bool removeAndFlag); /* - * prototypes for preptlist.h + * prototypes for preptlist.c */ extern List *preprocess_targetlist(List *tlist, int command_type, Index result_relation, List *range_table); +/* + * prototypes for prepunion.c + */ extern List *find_all_inheritors(List *unexamined_relids, List *examined_relids); extern int first_inherit_rt_entry(List *rangetable); extern Append *plan_union_queries(Query *parse); -extern Append *plan_inherit_queries(Query *parse, Index rt_index); +extern Append *plan_inherit_queries(Query *parse, List *tlist, Index rt_index); #endif /* PREP_H */