Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables) - Mailing list pgsql-hackers

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