Message-ID: <53E214C8.3070704@lab.ntt.co.jp>
Date: Wed, 06 Aug 2014 20:43:04 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.0;
 rv:24.0) Gecko/20100101 Thunderbird/24.6.0
MIME-Version: 1.0
To: Tom Lane <tgl@sss.pgh.pa.us>
CC: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>,
 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
 Shigeru Hanada <shigeru.hanada@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: inherit support for foreign tables
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>
In-Reply-To: <53B2188B.4090302@lab.ntt.co.jp>
Content-Type: multipart/mixed; boundary="------------020806060204020506010605"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

This is a multi-part message in MIME format.
--------------020806060204020506010605
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit

(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

--------------020806060204020506010605
Content-Type: text/x-diff;
 name="attr_needed-v1.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="attr_needed-v1.patch"

*** 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 */

--------------020806060204020506010605
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--------------020806060204020506010605--