From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: Shigeru Hanada <shigeru.hanada@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: inherit support for foreign tables
Date: Wed, 06 Aug 2014 20:43:04 +0900
Message-ID: <53E214C8.3070704@lab.ntt.co.jp> (raw)
In-Reply-To: <53B2188B.4090302@lab.ntt.co.jp>
References: <53393E18.3080901@lab.ntt.co.jp>
<533C01C7.6030809@lab.ntt.co.jp>
<53706FB6.6000403@lab.ntt.co.jp>
<20140620.170406.123403564.horiguchi.kyotaro@lab.ntt.co.jp>
<CAFjFpRcG03b3UZSXTKmro+72qYTdBfxoWCaEMgZBxY1QKyJrbQ@mail.gmail.com>
<53A92924.90702@lab.ntt.co.jp>
<53B10914.2010504@lab.ntt.co.jp>
<3492.1404136084@sss.pgh.pa.us>
<53B2188B.4090302@lab.ntt.co.jp>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
(2014/07/01 11:10), Etsuro Fujita wrote:
> (2014/06/30 22:48), Tom Lane wrote:
>> Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
>>> Done. I think this is because create_foreignscan_plan() makes reference
>>> to attr_needed, which isn't computed for inheritance children.
>>
>> 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.
>
> +1 for calculating attr_needed for child rels. (I was wondering too.)
>
> I'll create a separate patch for it.
Attached is a WIP patch for that. The following functions have been
changed to refer to attr_needed:
* check_index_only()
* remove_unused_subquery_outputs()
* check_selective_binary_conversion()
I'll add this to the upcoming commitfest. If anyone has any time to
glance at it before then, that would be a great help.
Thanks,
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[text/x-diff] attr_needed-v1.patch (9.2K, ../53E214C8.3070704@lab.ntt.co.jp/2-attr_needed-v1.patch)
download | inline diff:
*** 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,811 ----
}
/* 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, childRTE, 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,2205 ----
* 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,1781 ----
* way out.
*/
! /* Collect 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,122 ----
List *translated_vars);
static Node *adjust_appendrel_attrs_mutator(Node *node,
adjust_appendrel_attrs_context *context);
+ static void adjust_appendrel_attr_needed_inh(RelOptInfo *oldrel,+ RelOptInfo *newrel,+ RangeTblEntry *newrte,+ AppendRelInfo *appinfo);+ static void adjust_appendrel_attr_needed_setop(RelOptInfo *oldrel,+ RelOptInfo *newrel,+ RangeTblEntry *newrte,+ 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,
--- 1875,2002 ----
}
/*
+ * 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 *oldrel,+ RelOptInfo *newrel,+ RangeTblEntry *newrte,+ AppendRelInfo *appinfo)+ {+ if (OidIsValid(appinfo->parent_reloid))+ adjust_appendrel_attr_needed_inh(oldrel, newrel, newrte, appinfo);+ else+ adjust_appendrel_attr_needed_setop(oldrel, newrel, newrte, appinfo);+ }+ + static void+ adjust_appendrel_attr_needed_inh(RelOptInfo *oldrel,+ RelOptInfo *newrel,+ RangeTblEntry *newrte,+ AppendRelInfo *appinfo)+ {+ int i;+ ListCell *lc;+ + Assert(newrte->rtekind == RTE_RELATION);+ + /*+ * System attributes and whole-row Vars have the same numbers in all tables.+ */+ for (i = newrel->min_attr; i <= InvalidAttrNumber; i++)+ newrel->attr_needed[i - newrel->min_attr] =+ adjust_relid_set(oldrel->attr_needed[i - newrel->min_attr],+ appinfo->parent_relid, appinfo->child_relid);+ + /*+ * Compute user attributes' attr_needed through the translated_vars mapping.+ */+ i = InvalidAttrNumber;+ foreach(lc, appinfo->translated_vars)+ {+ Var *var = (Var *) lfirst(lc);+ int oldattoff;+ + i++;+ if (var == NULL) /* ignore dropped columns */+ continue;+ Assert(IsA(var, Var));+ + if (!bms_is_empty(oldrel->attr_needed[i - newrel->min_attr]))+ newrel->attr_needed[var->varattno - oldrel->min_attr] =+ adjust_relid_set(oldrel->attr_needed[i - newrel->min_attr],+ appinfo->parent_relid, appinfo->child_relid);+ }+ }+ + static void+ adjust_appendrel_attr_needed_setop(RelOptInfo *oldrel,+ RelOptInfo *newrel,+ RangeTblEntry *newrte,+ AppendRelInfo *appinfo)+ {+ int i;+ ListCell *lc;+ bool whole_row;+ + if (newrte->rtekind != RTE_RELATION)+ {+ for (i = oldrel->min_attr; i <= oldrel->max_attr; i++)+ newrel->attr_needed[i] = adjust_relid_set(oldrel->attr_needed[i],+ appinfo->parent_relid,+ appinfo->child_relid);+ return;+ }+ + /*+ * Compute the child's attr_needed by transforming the parent's attr_needed+ * through the translated_vars mapping of the specified AppendRelInfo.+ */+ + /* Check if parent has whole-row reference */+ whole_row = !bms_is_empty(oldrel->attr_needed[InvalidAttrNumber]);+ + i = InvalidAttrNumber;+ foreach(lc, appinfo->translated_vars)+ {+ Node *node = (Node *) lfirst(lc);+ bool result;+ + i++;+ if (whole_row || !bms_is_empty(oldrel->attr_needed[i]))+ {+ Bitmapset *attrs_used = NULL;+ Bitmapset *tmpset = NULL;+ int j;+ + pull_varattnos(node, newrel->relid, &attrs_used);+ tmpset = bms_copy(attrs_used);+ while ((j = bms_first_member(tmpset)) >= 0)+ {+ if (whole_row)+ newrel->attr_needed[j - 1] =+ bms_add_members(newrel->attr_needed[j - 1],+ oldrel->attr_needed[InvalidAttrNumber]);+ if (!bms_is_empty(oldrel->attr_needed[i]))+ newrel->attr_needed[j - 1] =+ bms_add_members(newrel->attr_needed[j - 1],+ oldrel->attr_needed[i]);+ }+ bms_free(tmpset);+ bms_free(attrs_used);+ }+ }+ + /* Adjust attr_needed's relid sets */+ for (i = newrel->min_attr; i <= newrel->max_attr; i++)+ newrel->attr_needed[i - newrel->min_attr] =+ adjust_relid_set(newrel->attr_needed[i - newrel->min_attr],+ 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,66 ----
extern Node *adjust_appendrel_attrs(PlannerInfo *root, Node *node,
AppendRelInfo *appinfo);
+ extern void adjust_appendrel_attr_needed(RelOptInfo *oldrel,+ RelOptInfo *newrel,+ RangeTblEntry *newrte,+ AppendRelInfo *appinfo);+
#endif /* PREP_H */
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: fujita.etsuro@lab.ntt.co.jp, tgl@sss.pgh.pa.us, ashutosh.bapat@enterprisedb.com, horiguchi.kyotaro@lab.ntt.co.jp, shigeru.hanada@gmail.com
Subject: Re: inherit support for foreign tables
In-Reply-To: <53E214C8.3070704@lab.ntt.co.jp>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox