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

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