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
