Hi Fujita-san, I reviewed the v4 patch, and here are some comments from me.
* After applying this patch, pull_varattnos() should not called in unnecessary places. For developers who want list of columns-to-be-processed for some purpose, it would be nice to mention when they should use pull_varattnos() and when they should not, maybe at the comments of pull_varattnos() implementation. * deparseReturningList() and postgresGetForeignRelSize() in contrib/postgres_fdw/ also call pull_varattnos() to determine which column to be in the SELECT clause of remote query. Shouldn't these be replaced in the same manner? Other pull_varattnos() calls are for restrictions, so IIUC they can't be replaced. * Through this review I thought up that lazy evaluation approach might fit attr_needed. I mean that we leave attr_needed for child relations empty, and fill it at the first request for it. This would avoid useless computation of attr_needed for child relations, though this idea has been considered but thrown away before... 2014-08-20 18:55 GMT+09:00 Etsuro Fujita <fujita.ets...@lab.ntt.co.jp>: > Hi Ashutish, > > > (2014/08/14 22:30), Ashutosh Bapat wrote: >> >> On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita >> <fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@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 -- Shigeru HANADA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
