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