Hi Ashutish,
(2014/08/14 22:30), Ashutosh Bapat wrote:
On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@lab.ntt.co.jp>> wrote:
(2014/08/08 18:51), Etsuro Fujita wrote:
>>> (2014/06/30 22:48), Tom Lane wrote:
>>>> I wonder whether it isn't time to change that. It was coded
like that
>>>> originally only because calculating the values would've been a
waste of
>>>> cycles at the time. But this is at least the third place
where it'd be
>>>> useful to have attr_needed for child rels.
> I've revised the patch.
There was a problem with the previous patch, which will be described
below. Attached is the updated version of the patch addressing that.
Here are some more comments
Thank you for the further review!
attr_needed also has the attributes used in the restriction clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().
IIUC, I think it's *necessary* to do that in those functions since the
attributes used in the restriction clauses in baserestrictinfo are not
added to attr_needed due the following code in distribute_qual_to_rels.
/*
* If it's a join clause (either naturally, or because delayed by
* outer-join rules), add vars used in the clause to targetlists of
their
* relations, so that they will be emitted by the plan nodes that scan
* those relations (else they won't be available at the join node!).
*
* Note: if the clause gets absorbed into an EquivalenceClass then this
* may be unnecessary, but for now we have to do it to cover the case
* where the EC becomes ec_broken and we end up reinserting the
original
* clauses into the plan.
*/
if (bms_membership(relids) == BMS_MULTIPLE)
{
List *vars = pull_var_clause(clause,
PVC_RECURSE_AGGREGATES,
PVC_INCLUDE_PLACEHOLDERS);
add_vars_to_targetlist(root, vars, relids, false);
list_free(vars);
}
Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in RelOptInfo
443 AttrNumber min_attr; /* smallest attrno of rel (often
<0) */
444 AttrNumber max_attr; /* largest attrno of rel */
445 Relids *attr_needed; /* array indexed [min_attr ..
max_attr] */
Good point! Attached is the revised version of the patch.
Thanks,
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***************
*** 799,806 **** check_selective_binary_conversion(RelOptInfo *baserel,
}
/* Collect all the attributes needed for joins or final output. */
! pull_varattnos((Node *) baserel->reltargetlist, baserel->relid,
! &attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel->baserestrictinfo)
--- 799,810 ----
}
/* Collect all the attributes needed for joins or final output. */
! for (i = baserel->min_attr; i <= baserel->max_attr; i++)
! {
! if (!bms_is_empty(baserel->attr_needed[i - baserel->min_attr]))
! attrs_used = bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel->baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***************
*** 577,588 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
childrel->has_eclass_joins = rel->has_eclass_joins;
/*
! * 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.
*/
/*
* Compute the child's size.
--- 577,585 ----
childrel->has_eclass_joins = rel->has_eclass_joins;
/*
! * Compute the child's attr_needed.
*/
+ adjust_appendrel_attr_needed(rel, childrel, appinfo);
/*
* Compute the child's size.
***************
*** 2173,2178 **** remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176 ----
{
Bitmapset *attrs_used = NULL;
ListCell *lc;
+ int i;
/*
* Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***************
*** 2193,2204 **** remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
* Collect a bitmap of all the output column numbers used by the upper
* query.
*
! * Add all the attributes needed for joins or final output. Note: we must
! * look at reltargetlist, not the attr_needed data, because attr_needed
! * isn't computed for inheritance child rels, cf set_append_rel_size().
! * (XXX might be worth changing that sometime.)
*/
! pull_varattnos((Node *) rel->reltargetlist, rel->relid, &attrs_used);
/* Add all the attributes used by un-pushed-down restriction clauses. */
foreach(lc, rel->baserestrictinfo)
--- 2191,2204 ----
* Collect a bitmap of all the output column numbers used by the upper
* query.
*
! * Add all the attributes needed for joins or final output.
*/
! for (i = rel->min_attr; i <= rel->max_attr; i++)
! {
! if (!bms_is_empty(rel->attr_needed[i - rel->min_attr]))
! attrs_used = bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by un-pushed-down restriction clauses. */
foreach(lc, rel->baserestrictinfo)
*** a/src/backend/optimizer/path/indxpath.c
--- b/src/backend/optimizer/path/indxpath.c
***************
*** 1768,1779 **** check_index_only(RelOptInfo *rel, IndexOptInfo *index)
* way out.
*/
! /*
! * Add all the attributes needed for joins or final output. Note: we must
! * look at reltargetlist, not the attr_needed data, because attr_needed
! * isn't computed for inheritance child rels.
! */
! pull_varattnos((Node *) rel->reltargetlist, rel->relid, &attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, rel->baserestrictinfo)
--- 1768,1780 ----
* way out.
*/
! /* Add all the attributes needed for joins or final output. */
! for (i = rel->min_attr; i <= rel->max_attr; i++)
! {
! if (!bms_is_empty(rel->attr_needed[i - rel->min_attr]))
! attrs_used = bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, rel->baserestrictinfo)
*** a/src/backend/optimizer/prep/prepunion.c
--- b/src/backend/optimizer/prep/prepunion.c
***************
*** 109,114 **** static Bitmapset *translate_col_privs(const Bitmapset *parent_privs,
--- 109,120 ----
List *translated_vars);
static Node *adjust_appendrel_attrs_mutator(Node *node,
adjust_appendrel_attrs_context *context);
+ static void adjust_appendrel_attr_needed_inh(RelOptInfo *parent_rel,
+ RelOptInfo *child_rel,
+ AppendRelInfo *appinfo);
+ static void adjust_appendrel_attr_needed_setop(RelOptInfo *parent_rel,
+ RelOptInfo *child_rel,
+ AppendRelInfo *appinfo);
static Relids adjust_relid_set(Relids relids, Index oldrelid, Index newrelid);
static List *adjust_inherited_tlist(List *tlist,
AppendRelInfo *context);
***************
*** 1867,1872 **** adjust_appendrel_attrs_mutator(Node *node,
--- 1873,2016 ----
}
/*
+ * adjust_appendrel_attr_needed
+ * Compute the child's attr_needed by transforming the parent's attr_needed
+ * through the translated_vars mapping of the specified AppendRelInfo.
+ */
+ void
+ adjust_appendrel_attr_needed(RelOptInfo *parent_rel,
+ RelOptInfo *child_rel,
+ AppendRelInfo *appinfo)
+ {
+ if (OidIsValid(appinfo->parent_reloid))
+ adjust_appendrel_attr_needed_inh(parent_rel, child_rel, appinfo);
+ else
+ adjust_appendrel_attr_needed_setop(parent_rel, child_rel, appinfo);
+ }
+
+ /*
+ * adjust_appendrel_attr_needed for the inheritance case
+ */
+ static void
+ adjust_appendrel_attr_needed_inh(RelOptInfo *parent_rel,
+ RelOptInfo *child_rel,
+ AppendRelInfo *appinfo)
+ {
+ int attno;
+ ListCell *lc;
+
+ Assert(appinfo->parent_relid == parent_rel->relid);
+ Assert(appinfo->child_relid == child_rel->relid);
+
+ /*
+ * System attributes and whole-row Vars have the same numbers in all tables.
+ */
+ for (attno = parent_rel->min_attr; attno <= InvalidAttrNumber; attno++)
+ {
+ int pndx = attno - FirstLowInvalidHeapAttributeNumber - 1;
+
+ child_rel->attr_needed[pndx] =
+ adjust_relid_set(parent_rel->attr_needed[pndx],
+ appinfo->parent_relid, appinfo->child_relid);
+ }
+
+ /*
+ * Compute user attributes' attr_needed through the translated_vars mapping.
+ */
+ attno = InvalidAttrNumber;
+ foreach(lc, appinfo->translated_vars)
+ {
+ Var *var = (Var *) lfirst(lc);
+ int pndx;
+ int cndx;
+
+ attno++;
+ if (var == NULL) /* ignore dropped columns */
+ continue;
+ Assert(IsA(var, Var));
+
+ pndx = attno - FirstLowInvalidHeapAttributeNumber - 1;
+ cndx = var->varattno - FirstLowInvalidHeapAttributeNumber - 1;
+ child_rel->attr_needed[cndx] =
+ adjust_relid_set(parent_rel->attr_needed[pndx],
+ appinfo->parent_relid, appinfo->child_relid);
+ }
+ }
+
+ /*
+ * adjust_appendrel_attr_needed for the UNION ALL case
+ *
+ * Note: pull_up_subqueries might have changed the mapping so that their
+ * expansions contain arbitrary expressions. So we must cope with such.
+ */
+ static void
+ adjust_appendrel_attr_needed_setop(RelOptInfo *parent_rel,
+ RelOptInfo *child_rel,
+ AppendRelInfo *appinfo)
+ {
+ bool whole_row;
+ int attno;
+ ListCell *lc;
+
+ Assert(appinfo->parent_relid == parent_rel->relid);
+ Assert(appinfo->child_relid == child_rel->relid);
+
+ /* Check if parent has whole-row reference */
+ whole_row = !bms_is_empty(parent_rel->attr_needed[InvalidAttrNumber]);
+
+ attno = InvalidAttrNumber;
+ foreach(lc, appinfo->translated_vars)
+ {
+ Node *node = (Node *) lfirst(lc);
+ int pndx;
+ bool need_attr;
+
+ attno++;
+ pndx = attno;
+ need_attr = !bms_is_empty(parent_rel->attr_needed[pndx]);
+ if (need_attr || whole_row)
+ {
+ Bitmapset *attrs_used = NULL;
+ Bitmapset *tmpset = NULL;
+ int i;
+
+ pull_varattnos(node, child_rel->relid, &attrs_used);
+ tmpset = bms_copy(attrs_used);
+ while ((i = bms_first_member(tmpset)) >= 0)
+ {
+ int cndx;
+
+ if (child_rel->rtekind == RTE_RELATION)
+ cndx = i - 1;
+ else
+ cndx = i + FirstLowInvalidHeapAttributeNumber;
+
+ if (need_attr)
+ child_rel->attr_needed[cndx] =
+ bms_add_members(child_rel->attr_needed[cndx],
+ parent_rel->attr_needed[pndx]);
+ if (whole_row)
+ child_rel->attr_needed[cndx] =
+ bms_add_members(child_rel->attr_needed[cndx],
+ parent_rel->attr_needed[InvalidAttrNumber]);
+ }
+ bms_free(tmpset);
+ bms_free(attrs_used);
+ }
+ }
+
+ /* Fix attr_needed's relid sets */
+ for (attno = child_rel->min_attr; attno <= child_rel->max_attr; attno++)
+ {
+ int cndx = attno - child_rel->min_attr;
+
+ child_rel->attr_needed[cndx] =
+ adjust_relid_set(child_rel->attr_needed[cndx],
+ appinfo->parent_relid, appinfo->child_relid);
+ }
+ }
+
+ /*
* Substitute newrelid for oldrelid in a Relid set
*/
static Relids
*** a/src/include/optimizer/prep.h
--- b/src/include/optimizer/prep.h
***************
*** 58,61 **** extern void expand_inherited_tables(PlannerInfo *root);
--- 58,65 ----
extern Node *adjust_appendrel_attrs(PlannerInfo *root, Node *node,
AppendRelInfo *appinfo);
+ extern void adjust_appendrel_attr_needed(RelOptInfo *parent_rel,
+ RelOptInfo *child_rel,
+ AppendRelInfo *appinfo);
+
#endif /* PREP_H */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
