Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables) - Mailing list pgsql-hackers
From | Ashutosh Bapat |
---|---|
Subject | Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables) |
Date | |
Msg-id | CAFjFpRf3XZCvJH-3N93cqPQJrm8z1inH0mM-5E6DSekZgqGzYQ@mail.gmail.com Whole thread Raw |
In response to | Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables) (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>) |
List | pgsql-hackers |
On Thu, Aug 21, 2014 at 3:00 PM, Etsuro Fujita <fujita.etsuro@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.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@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:<mailto:fujita.etsuro@lab.ntt.__co.jp
On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp
<mailto:fujita.etsuro@lab.ntt.co.jp>
<mailto:fujita.etsuro@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 commentsattr_needed also has the attributes used in the restrictioncheck_selective_binary___conversion(),
clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
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_neededcorresponds 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] */I've already done that: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?
https://commitfest.postgresql.org/action/patch_view?id=1529
Many thanks!From my side, the review is done, it should be marked "ready for
committer", unless somebody else wants to review.
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
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
pgsql-hackers by date: