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

On Thu, Aug 21, 2014 at 3:00 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

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