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

(2014/08/21 13:21), Ashutosh Bapat wrote:
> On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
> <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)__co(dot)jp
> <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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