Hi,


On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita <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.
>
> > 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

Reply via email to