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

On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> Hi Ashutish,
>
>
> (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>> 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