(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:
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! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers