Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Etsuro Fujita]

(2014/08/27 11:06), Tom Lane wrote:
> Etsuro Fujita  writes:
>> (2014/08/27 3:27), Tom Lane wrote:
>>> I looked this over, and TBH I'm rather disappointed.  The patch adds
>>> 150 lines of dubiously-correct code in order to save ... uh, well,
> 
>> Just for my study, could you tell me why you think that the code is
>> "dubiously-correct"?
> 
> It might be fine; I did not actually review the new
> adjust_appendrel_attr_needed code in any detail.  What's scaring me off it
> is (1) it's a lot longer and more complicated than I'd thought it would
> be, and (2) you already made several bug fixes in it, which is often an
> indicator that additional problems lurk.

Okay.

> It's possible there's some other, simpler, way to compute child
> attr_needed arrays that would resolve (1) and (2).  However, even if we
> had a simple and obviously-correct way to do that, it still seems like
> there's not very much benefit to be had after all.  So my thought that
> this would be worth doing seems wrong, and I must apologize to you for
> sending you chasing down a dead end :-(

Please don't worry yourself about that!

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-hackers

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Tom Lane]

Etsuro Fujita  writes:
> (2014/08/27 3:27), Tom Lane wrote:
>> I looked this over, and TBH I'm rather disappointed.  The patch adds
>> 150 lines of dubiously-correct code in order to save ... uh, well,

> Just for my study, could you tell me why you think that the code is
> "dubiously-correct"?

It might be fine; I did not actually review the new
adjust_appendrel_attr_needed code in any detail.  What's scaring me off it
is (1) it's a lot longer and more complicated than I'd thought it would
be, and (2) you already made several bug fixes in it, which is often an
indicator that additional problems lurk.

It's possible there's some other, simpler, way to compute child
attr_needed arrays that would resolve (1) and (2).  However, even if we
had a simple and obviously-correct way to do that, it still seems like
there's not very much benefit to be had after all.  So my thought that
this would be worth doing seems wrong, and I must apologize to you for
sending you chasing down a dead end :-(

regards, tom lane


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

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Etsuro Fujita]

(2014/08/27 3:27), Tom Lane wrote:
> Etsuro Fujita  writes:
>> [ attr_needed-v4.patch ]
> 
> I looked this over, and TBH I'm rather disappointed.  The patch adds
> 150 lines of dubiously-correct code in order to save ... uh, well,

Just for my study, could you tell me why you think that the code is
"dubiously-correct"?

> Considering that all the
> places that are doing this then proceed to use pull_varattnos to add on
> attnos from the restriction clauses, it seems like using pull_varattnos
> on the reltargetlist isn't such a bad thing after all.

I agree with you on that point.

> So I'm inclined to reject this.  It seemed like a good idea in the
> abstract, but the concrete result isn't very attractive, and doesn't
> seem like an improvement over what we have.

Okay.  I'll withdraw the patch.

Thank you for taking the time to review the patch!

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-hackers

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Tom Lane]

Etsuro Fujita  writes:
> [ attr_needed-v4.patch ]

I looked this over, and TBH I'm rather disappointed.  The patch adds
150 lines of dubiously-correct code in order to save ... uh, well,
actually it *adds* code, because the places that are supposedly getting
a benefit are changed like this:

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

That's not simpler, it's not easier to understand, and it's probably not
faster either.  We could address some of those complaints by encapsulating
the above loop into a utility function, but still, I come away with the
feeling that it's not worth changing this.  Considering that all the
places that are doing this then proceed to use pull_varattnos to add on
attnos from the restriction clauses, it seems like using pull_varattnos
on the reltargetlist isn't such a bad thing after all.

So I'm inclined to reject this.  It seemed like a good idea in the
abstract, but the concrete result isn't very attractive, and doesn't
seem like an improvement over what we have.

regards, tom lane


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

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Shigeru Hanada]

Hi Fujita-san,

I reviewed the v4 patch, and here are some comments from me.

* After applying this patch, pull_varattnos() should not called in
unnecessary places.  For developers who want list of
columns-to-be-processed for some purpose, it would be nice to mention
when they should use pull_varattnos() and when they should not, maybe
at the comments of pull_varattnos() implementation.

* deparseReturningList() and postgresGetForeignRelSize() in
contrib/postgres_fdw/ also call pull_varattnos() to determine which
column to be in the SELECT clause of remote query.  Shouldn't these be
replaced in the same manner?  Other pull_varattnos() calls are for
restrictions, so IIUC they can't be replaced.

* Through this review I thought up that lazy evaluation approach might
fit attr_needed.  I mean that we leave attr_needed for child relations
empty, and fill it at the first request for it.  This would avoid
useless computation of attr_needed for child relations, though this
idea has been considered but thrown away before...


2014-08-20 18:55 GMT+09:00 Etsuro Fujita :
> Hi Ashutish,
>
>
> (2014/08/14 22:30), Ashutosh Bapat wrote:
>>
>> On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
>> 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



-- 
Shigeru HANADA


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

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Ashutosh Bapat]

On Thu, Aug 21, 2014 at 3:00 PM, Etsuro Fujita 
wrote:

> (2014/08/21 13:21), Ashutosh Bapat wrote:
>
>> On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita
>> mailto:fujita.ets...@lab.ntt.co.jp>> wrote:
>>
>
>  Hi Ashutish,
>>
>
> I am sorry that I mistook your name's spelling.
>
>  (2014/08/14 22:30), Ashutosh Bapat wrote:
>>
>> On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
>> > 
>> >
>> >> 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.
>>
>
>   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
>>
>
>  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.
>>
>
>  That's right. Thanks for pointing that out.
>>
>
>  Although in case of RTE_RELATION, the 0th entry in attr_needed
>> corresponds to FirstLowInvalidHeapAttributeNu__mber + 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.
>>
>
>  If the patch is not in the commit-fest, can you please add it there?
>>
>
> I've already done that:
>
> https://commitfest.postgresql.org/action/patch_view?id=1529
>
>
>   From my side, the review is done, it should be marked "ready for
>> committer", unless somebody else wants to review.
>>
>
> Many thanks!
>
>
Thanks. Since, I haven't seen anybody else commenting here and I do not
have any further comments to make, I have marked it as "ready for
committer".


> Best regards,
> Etsuro Fujita
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Etsuro Fujita]

(2014/08/21 13:21), Ashutosh Bapat wrote:

On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita
mailto:fujita.ets...@lab.ntt.co.jp>> wrote:



Hi Ashutish,


I am sorry that I mistook your name's spelling.


(2014/08/14 22:30), Ashutosh Bapat wrote:

On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
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.



 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



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.



That's right. Thanks for pointing that out.



Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNu__mber + 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.



If the patch is not in the commit-fest, can you please add it there?


I've already done that:

https://commitfest.postgresql.org/action/patch_view?id=1529


 From my side, the review is done, it should be marked "ready for
committer", unless somebody else wants to review.


Many 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-hackers

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Ashutosh Bapat]

On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita 
wrote:

> Hi Ashutish,
>
>
> (2014/08/14 22:30), Ashutosh Bapat wrote:
>
>> On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
>> 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.
>
>
That's right. Thanks for pointing that out.


> /*
>  * 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.
>
>
If the patch is not in the commit-fest, can you please add it there? From
my side, the review is done, it should be marked "ready for committer",
unless somebody else wants to review.


>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Etsuro Fujita]

Hi Ashutish,

(2014/08/14 22:30), Ashutosh Bapat wrote:

On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
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
***
**

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Ashutosh Bapat]

Hi,



On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita  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.
>
> 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 for taking care of this one.

Here are some more comments

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().

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


> Thanks,
>
> Best regards,
> Etsuro Fujita
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Etsuro Fujita]

(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 changi