From 04366799694418ed899e95ce45143a699a75116e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 11 Nov 2008 18:13:32 +0000 Subject: [PATCH] Get rid of adjust_appendrel_attr_needed(), which has been broken ever since we extended the appendrel mechanism to support UNION ALL optimization. The reason nobody noticed was that we are not actually using attr_needed data for appendrel children; hence it seems more reasonable to rip it out than fix it. Back-patch to 8.2 because an Assert failure is possible in corner cases. Per examination of an example from Jim Nasby. In HEAD, also get rid of AppendRelInfo.col_mappings, which is quite inadequate to represent UNION ALL situations; depend entirely on translated_vars instead. --- src/backend/nodes/copyfuncs.c | 3 +- src/backend/nodes/equalfuncs.c | 3 +- src/backend/nodes/outfuncs.c | 3 +- src/backend/optimizer/path/allpaths.c | 14 ++-- src/backend/optimizer/prep/prepjointree.c | 29 ++++---- src/backend/optimizer/prep/prepunion.c | 114 ++++++------------------------ src/include/nodes/relation.h | 19 +---- src/include/optimizer/prep.h | 7 +- 8 files changed, 46 insertions(+), 146 deletions(-) diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index d56075e6bb..b26fb827aa 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -15,7 +15,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/nodes/copyfuncs.c,v 1.410 2008/10/31 08:39:20 heikki Exp $ + * $PostgreSQL: pgsql/src/backend/nodes/copyfuncs.c,v 1.411 2008/11/11 18:13:32 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -1640,7 +1640,6 @@ _copyAppendRelInfo(AppendRelInfo *from) COPY_SCALAR_FIELD(child_relid); COPY_SCALAR_FIELD(parent_reltype); COPY_SCALAR_FIELD(child_reltype); - COPY_NODE_FIELD(col_mappings); COPY_NODE_FIELD(translated_vars); COPY_SCALAR_FIELD(parent_reloid); diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index e17aff8922..01cb7d94c6 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -22,7 +22,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/nodes/equalfuncs.c,v 1.335 2008/10/31 08:39:20 heikki Exp $ + * $PostgreSQL: pgsql/src/backend/nodes/equalfuncs.c,v 1.336 2008/11/11 18:13:32 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -806,7 +806,6 @@ _equalAppendRelInfo(AppendRelInfo *a, AppendRelInfo *b) COMPARE_SCALAR_FIELD(child_relid); COMPARE_SCALAR_FIELD(parent_reltype); COMPARE_SCALAR_FIELD(child_reltype); - COMPARE_NODE_FIELD(col_mappings); COMPARE_NODE_FIELD(translated_vars); COMPARE_SCALAR_FIELD(parent_reloid); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index d1130760f9..c8febd273b 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/nodes/outfuncs.c,v 1.343 2008/10/21 20:42:52 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/nodes/outfuncs.c,v 1.344 2008/11/11 18:13:32 tgl Exp $ * * NOTES * Every node type that can appear in stored rules' parsetrees *must* @@ -1628,7 +1628,6 @@ _outAppendRelInfo(StringInfo str, AppendRelInfo *node) WRITE_UINT_FIELD(child_relid); WRITE_OID_FIELD(parent_reltype); WRITE_OID_FIELD(child_reltype); - WRITE_NODE_FIELD(col_mappings); WRITE_NODE_FIELD(translated_vars); WRITE_OID_FIELD(parent_reloid); } diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 7d6a3b8d6b..4d868570bc 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/optimizer/path/allpaths.c,v 1.175 2008/10/21 20:42:52 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/optimizer/path/allpaths.c,v 1.176 2008/11/11 18:13:32 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -382,14 +382,12 @@ set_append_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, } /* - * Copy the parent's attr_needed data as well, with appropriate - * adjustment of relids and attribute numbers. + * Note: we could compute appropriate attr_needed data for the + * child's variables, by transforming the parent's attr_needed + * through the translated_vars mapping. However, currently there's + * no need because attr_needed is only examined for base relations + * not otherrels. So we just leave the child's attr_needed empty. */ - pfree(childrel->attr_needed); - childrel->attr_needed = - adjust_appendrel_attr_needed(rel, appinfo, - childrel->min_attr, - childrel->max_attr); /* * Compute the child's access paths, and add the cheapest one to the diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index b15a0e5dd4..b5882a5771 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -16,7 +16,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/optimizer/prep/prepjointree.c,v 1.58 2008/10/22 20:17:52 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/optimizer/prep/prepjointree.c,v 1.59 2008/11/11 18:13:32 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -54,9 +54,8 @@ static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode, static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex, Query *setOpQuery, int childRToffset); -static void make_setop_translation_lists(Query *query, - Index newvarno, - List **col_mappings, List **translated_vars); +static void make_setop_translation_list(Query *query, Index newvarno, + List **translated_vars); static bool is_simple_subquery(Query *subquery); static bool is_simple_union_all(Query *subquery); static bool is_simple_union_all_recurse(Node *setOp, Query *setOpQuery, @@ -839,9 +838,8 @@ pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex, appinfo->child_relid = childRTindex; appinfo->parent_reltype = InvalidOid; appinfo->child_reltype = InvalidOid; - make_setop_translation_lists(setOpQuery, childRTindex, - &appinfo->col_mappings, - &appinfo->translated_vars); + make_setop_translation_list(setOpQuery, childRTindex, + &appinfo->translated_vars); appinfo->parent_reloid = InvalidOid; root->append_rel_list = lappend(root->append_rel_list, appinfo); @@ -874,17 +872,16 @@ pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex, } /* - * make_setop_translation_lists - * Build the lists of translations from parent Vars to child Vars for - * a UNION ALL member. We need both a column number mapping list - * and a list of Vars representing the child columns. + * make_setop_translation_list + * Build the list of translations from parent Vars to child Vars for + * a UNION ALL member. (At this point it's just a simple list of + * referencing Vars, but if we succeed in pulling up the member + * subquery, the Vars will get replaced by pulled-up expressions.) */ static void -make_setop_translation_lists(Query *query, - Index newvarno, - List **col_mappings, List **translated_vars) +make_setop_translation_list(Query *query, Index newvarno, + List **translated_vars) { - List *numbers = NIL; List *vars = NIL; ListCell *l; @@ -895,7 +892,6 @@ make_setop_translation_lists(Query *query, if (tle->resjunk) continue; - numbers = lappend_int(numbers, tle->resno); vars = lappend(vars, makeVar(newvarno, tle->resno, exprType((Node *) tle->expr), @@ -903,7 +899,6 @@ make_setop_translation_lists(Query *query, 0)); } - *col_mappings = numbers; *translated_vars = vars; } diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index dd7f2f28e0..c2113cb266 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -22,7 +22,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.160 2008/10/22 20:17:52 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.161 2008/11/11 18:13:32 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -91,11 +91,10 @@ static List *generate_append_tlist(List *colTypes, bool flag, static List *generate_setop_grouplist(SetOperationStmt *op, List *targetlist); static void expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti); -static void make_inh_translation_lists(Relation oldrelation, - Relation newrelation, - Index newvarno, - List **col_mappings, - List **translated_vars); +static void make_inh_translation_list(Relation oldrelation, + Relation newrelation, + Index newvarno, + List **translated_vars); static Node *adjust_appendrel_attrs_mutator(Node *node, AppendRelInfo *context); static Relids adjust_relid_set(Relids relids, Index oldrelid, Index newrelid); @@ -1279,9 +1278,8 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti) appinfo->child_relid = childRTindex; appinfo->parent_reltype = oldrelation->rd_rel->reltype; appinfo->child_reltype = newrelation->rd_rel->reltype; - make_inh_translation_lists(oldrelation, newrelation, childRTindex, - &appinfo->col_mappings, - &appinfo->translated_vars); + make_inh_translation_list(oldrelation, newrelation, childRTindex, + &appinfo->translated_vars); appinfo->parent_reloid = parentOID; appinfos = lappend(appinfos, appinfo); @@ -1316,19 +1314,17 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti) } /* - * make_inh_translation_lists - * Build the lists of translations from parent Vars to child Vars for - * an inheritance child. We need both a column number mapping list - * and a list of Vars representing the child columns. + * make_inh_translation_list + * Build the list of translations from parent Vars to child Vars for + * an inheritance child. * * For paranoia's sake, we match type as well as attribute name. */ static void -make_inh_translation_lists(Relation oldrelation, Relation newrelation, - Index newvarno, - List **col_mappings, List **translated_vars) +make_inh_translation_list(Relation oldrelation, Relation newrelation, + Index newvarno, + List **translated_vars) { - List *numbers = NIL; List *vars = NIL; TupleDesc old_tupdesc = RelationGetDescr(oldrelation); TupleDesc new_tupdesc = RelationGetDescr(newrelation); @@ -1347,8 +1343,7 @@ make_inh_translation_lists(Relation oldrelation, Relation newrelation, att = old_tupdesc->attrs[old_attno]; if (att->attisdropped) { - /* Just put 0/NULL into this list entry */ - numbers = lappend_int(numbers, 0); + /* Just put NULL into this list entry */ vars = lappend(vars, NULL); continue; } @@ -1362,7 +1357,6 @@ make_inh_translation_lists(Relation oldrelation, Relation newrelation, */ if (oldrelation == newrelation) { - numbers = lappend_int(numbers, old_attno + 1); vars = lappend(vars, makeVar(newvarno, (AttrNumber) (old_attno + 1), atttypid, @@ -1405,7 +1399,6 @@ make_inh_translation_lists(Relation oldrelation, Relation newrelation, elog(ERROR, "attribute \"%s\" of relation \"%s\" does not match parent's type", attname, RelationGetRelationName(newrelation)); - numbers = lappend_int(numbers, new_attno + 1); vars = lappend(vars, makeVar(newvarno, (AttrNumber) (new_attno + 1), atttypid, @@ -1413,7 +1406,6 @@ make_inh_translation_lists(Relation oldrelation, Relation newrelation, 0)); } - *col_mappings = numbers; *translated_vars = vars; } @@ -1683,70 +1675,6 @@ adjust_relid_set(Relids relids, Index oldrelid, Index newrelid) } /* - * adjust_appendrel_attr_needed - * Adjust an attr_needed[] array to reference a member rel instead of - * the original appendrel - * - * oldrel: source of data (we use the attr_needed, min_attr, max_attr fields) - * appinfo: supplies parent_relid, child_relid, col_mappings - * new_min_attr, new_max_attr: desired bounds of new attr_needed array - * - * The relid sets are adjusted by substituting child_relid for parent_relid. - * (NOTE: oldrel is not necessarily the parent_relid relation!) We are also - * careful to map attribute numbers within the array properly. User - * attributes have to be mapped through col_mappings, but system attributes - * and whole-row references always have the same attno. - * - * Returns a palloc'd array with the specified bounds - */ -Relids * -adjust_appendrel_attr_needed(RelOptInfo *oldrel, AppendRelInfo *appinfo, - AttrNumber new_min_attr, AttrNumber new_max_attr) -{ - Relids *new_attr_needed; - Index parent_relid = appinfo->parent_relid; - Index child_relid = appinfo->child_relid; - int parent_attr; - ListCell *lm; - - /* Create empty result array */ - new_attr_needed = (Relids *) - palloc0((new_max_attr - new_min_attr + 1) * sizeof(Relids)); - /* Process user attributes, with appropriate attno mapping */ - parent_attr = 1; - foreach(lm, appinfo->col_mappings) - { - int child_attr = lfirst_int(lm); - - if (child_attr > 0) - { - Relids attrneeded; - - Assert(parent_attr <= oldrel->max_attr); - Assert(child_attr <= new_max_attr); - attrneeded = oldrel->attr_needed[parent_attr - oldrel->min_attr]; - attrneeded = adjust_relid_set(attrneeded, - parent_relid, child_relid); - new_attr_needed[child_attr - new_min_attr] = attrneeded; - } - parent_attr++; - } - /* Process system attributes, including whole-row references */ - Assert(new_min_attr <= oldrel->min_attr); - for (parent_attr = oldrel->min_attr; parent_attr <= 0; parent_attr++) - { - Relids attrneeded; - - attrneeded = oldrel->attr_needed[parent_attr - oldrel->min_attr]; - attrneeded = adjust_relid_set(attrneeded, - parent_relid, child_relid); - new_attr_needed[parent_attr - new_min_attr] = attrneeded; - } - - return new_attr_needed; -} - -/* * Adjust the targetlist entries of an inherited UPDATE operation * * The expressions have already been fixed, but we have to make sure that @@ -1778,24 +1706,24 @@ adjust_inherited_tlist(List *tlist, AppendRelInfo *context) foreach(tl, tlist) { TargetEntry *tle = (TargetEntry *) lfirst(tl); - int newattno; + Var *childvar; if (tle->resjunk) continue; /* ignore junk items */ - /* Look up the translation of this column */ + /* Look up the translation of this column: it must be a Var */ if (tle->resno <= 0 || - tle->resno > list_length(context->col_mappings)) + tle->resno > list_length(context->translated_vars)) elog(ERROR, "attribute %d of relation \"%s\" does not exist", tle->resno, get_rel_name(context->parent_reloid)); - newattno = list_nth_int(context->col_mappings, tle->resno - 1); - if (newattno <= 0) + childvar = (Var *) list_nth(context->translated_vars, tle->resno - 1); + if (childvar == NULL || !IsA(childvar, Var)) elog(ERROR, "attribute %d of relation \"%s\" does not exist", tle->resno, get_rel_name(context->parent_reloid)); - if (tle->resno != newattno) + if (tle->resno != childvar->varattno) { - tle->resno = newattno; + tle->resno = childvar->varattno; changed_it = true; } } diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h index 4ce13f808a..56074a469d 100644 --- a/src/include/nodes/relation.h +++ b/src/include/nodes/relation.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/nodes/relation.h,v 1.163 2008/10/22 20:17:52 tgl Exp $ + * $PostgreSQL: pgsql/src/include/nodes/relation.h,v 1.164 2008/11/11 18:13:32 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -1240,26 +1240,13 @@ typedef struct AppendRelInfo Oid child_reltype; /* OID of child's composite type */ /* - * The N'th element of this list is the integer column number of the child - * column corresponding to the N'th column of the parent. A list element - * is zero if it corresponds to a dropped column of the parent (this is - * only possible for inheritance cases, not UNION ALL). - */ - List *col_mappings; /* list of child attribute numbers */ - - /* * The N'th element of this list is a Var or expression representing the * child column corresponding to the N'th column of the parent. This is * used to translate Vars referencing the parent rel into references to * the child. A list element is NULL if it corresponds to a dropped * column of the parent (this is only possible for inheritance cases, not - * UNION ALL). - * - * This might seem redundant with the col_mappings data, but it is handy - * because flattening of sub-SELECTs that are members of a UNION ALL will - * cause changes in the expressions that need to be substituted for a - * parent Var. Adjusting this data structure lets us track what really - * needs to be substituted. + * UNION ALL). The list elements are always simple Vars for inheritance + * cases, but can be arbitrary expressions in UNION ALL cases. * * Notice we only store entries for user columns (attno > 0). Whole-row * Vars are special-cased, and system columns (attno < 0) need no special diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h index 9326623b91..b6651866e7 100644 --- a/src/include/optimizer/prep.h +++ b/src/include/optimizer/prep.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/optimizer/prep.h,v 1.62 2008/08/17 01:20:00 tgl Exp $ + * $PostgreSQL: pgsql/src/include/optimizer/prep.h,v 1.63 2008/11/11 18:13:32 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -51,9 +51,4 @@ extern void expand_inherited_tables(PlannerInfo *root); extern Node *adjust_appendrel_attrs(Node *node, AppendRelInfo *appinfo); -extern Relids *adjust_appendrel_attr_needed(RelOptInfo *oldrel, - AppendRelInfo *appinfo, - AttrNumber new_min_attr, - AttrNumber new_max_attr); - #endif /* PREP_H */ -- 2.11.0