On Thu, Aug 21, 2014 at 3:00 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote:
> (2014/08/21 13:21), Ashutosh Bapat wrote: > >> On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita >> <fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@lab.ntt.co.jp>> wrote: >> > > Hi Ashutish, >> > > I am sorry that I mistook your name's spelling. > > (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> >> <mailto: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. >> > > 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 >> > > 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. >> > > Although in case of RTE_RELATION, the 0th entry in attr_needed >> corresponds to FirstLowInvalidHeapAttributeNu__mber + 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? >> > > I've already done that: > > https://commitfest.postgresql.org/action/patch_view?id=1529 > > > From my side, the review is done, it should be marked "ready for >> committer", unless somebody else wants to review. >> > > Many thanks! > > Thanks. Since, I haven't seen anybody else commenting here and I do not have any further comments to make, I have marked it as "ready for committer". > Best regards, > Etsuro Fujita > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
