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

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables)
Date: 2014-08-20 09:55:08
Message-ID: 53F4707C.8030904@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

/*
* 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

Attachment Content-Type Size
attr_needed-v4.patch text/x-diff 9.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2014-08-20 10:17:06 Re: better atomics - v0.5
Previous Message Heikki Linnakangas 2014-08-20 09:53:51 Re: implement subject alternative names support for SSL connections