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

From Ashutosh Bapat
Subject Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables)
Date
Msg-id CAFjFpRf29PcD2SWZCLPpEWoRcAkA+MqHruff-cxDLkW2rkdDcA@mail.gmail.com
Whole thread Raw
In response to Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables)  (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
Responses Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables)
List pgsql-hackers



On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
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.


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

pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: TODO : Allow parallel cores to be used by vacuumdb [ WIP ]
Next
From: Michael Paquier
Date:
Subject: Re: Support for N synchronous standby servers