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

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 <fujita.etsuro@lab.ntt.co.jp>:
> Hi Ashutish,
>
>
> (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>> 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