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 | CAFjFpReaXpzyqot2p63+KyXAYB7SXu+P=re+zQ6kBFuKbRCKnA@mail.gmail.com Whole thread Raw |
In response to | Compute attr_needed for child relations (was Re: inherit support for foreign tables) (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>) |
Responses |
Re: Compute attr_needed for child relations (was Re: inherit
support for foreign tables)
|
List | pgsql-hackers |
Hi,
--
On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita <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.
> 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.
The previous patch doesn't cope with some UNION ALL cases properly. So,
e.g., the server will crash for the following query:
postgres=# create table ta1 (f1 int);
CREATE TABLE
postgres=# create table ta2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# create table tb1 (f1 int);
CREATE TABLE
postgres=# create table tb2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;
With the updated version, we get the right result:
postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;
QUERY PLAN
--------------------------------------------------------------------------------
Append (cost=0.00..0.05 rows=2 width=4)
-> Subquery Scan on ssa (cost=0.00..0.02 rows=1 width=4)
Output: ssa.f1
-> Limit (cost=0.00..0.01 rows=1 width=4)
Output: ta1.f1, (NULL::integer), (NULL::integer)
-> Seq Scan on public.ta1 (cost=0.00..34.00 rows=2400
width=4)
Output: ta1.f1, NULL::integer, NULL::integer
-> Subquery Scan on ssb (cost=0.00..0.02 rows=1 width=4)
Output: ssb.f1
-> Limit (cost=0.00..0.01 rows=1 width=4)
Output: tb1.f1, (NULL::integer), (NULL::integer)
-> Seq Scan on public.tb1 (cost=0.00..34.00 rows=2400
width=4)
Output: tb1.f1, NULL::integer, NULL::integer
Planning time: 0.453 ms
(14 rows)
While thinking to address this problem, Ashutosh also expressed concern
about the UNION ALL handling in the previous patch in a private email.
Thank you for the review, Ashutosh!
Thanks for taking care of this one.
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().
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] */
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().
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] */
Thanks,
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: