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

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