(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.
The previous patch doesn't cope with some UNION ALL cases properly. So,
e.g., the server will crash for the following query:
postgres=# create table ta1 (f1 int);
CREATE TABLE
postgres=# create table ta2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# create table tb1 (f1 int);
CREATE TABLE
postgres=# create table tb2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;
With the updated version, we get the right result:
postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;
QUERY PLAN
--------------------------------------------------------------------------------
Append (cost=0.00..0.05 rows=2 width=4)
-> Subquery Scan on ssa (cost=0.00..0.02 rows=1 width=4)
Output: ssa.f1
-> Limit (cost=0.00..0.01 rows=1 width=4)
Output: ta1.f1, (NULL::integer), (NULL::integer)
-> Seq Scan on public.ta1 (cost=0.00..34.00 rows=2400
width=4)
Output: ta1.f1, NULL::integer, NULL::integer
-> Subquery Scan on ssb (cost=0.00..0.02 rows=1 width=4)
Output: ssb.f1
-> Limit (cost=0.00..0.01 rows=1 width=4)
Output: tb1.f1, (NULL::integer), (NULL::integer)
-> Seq Scan on public.tb1 (cost=0.00..34.00 rows=2400
width=4)
Output: tb1.f1, NULL::integer, NULL::integer
Planning time: 0.453 ms
(14 rows)
While thinking to address this problem, Ashutosh also expressed concern
about the UNION ALL handling in the previous patch in a private email.
Thank you for the review, Ashutosh!
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,2017 ----
}
/*
+ * 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(child_rel->rtekind == RTE_RELATION);
+ 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 = FirstLowInvalidHeapAttributeNumber + 1; attno <= 0; 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
