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] */
 
Thanks,

Best regards,
Etsuro Fujita



--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pgsql-hackers by date:

Previous
From: Pavel Stehule
Date:
Subject: Re: pg_dump bug in 9.4beta2 and HEAD
Next
From: Alvaro Herrera
Date:
Subject: Re: pg_dump bug in 9.4beta2 and HEAD