|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)|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
(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(),
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
* 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
* clauses into the plan.
if (bms_membership(relids) == BMS_MULTIPLE)
List *vars = pull_var_clause(clause,
add_vars_to_targetlist(root, vars, relids, false);
> 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.
|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|