Re: Compute attr_needed for child relations (was Re: inherit support for foreign tables)

Hi,

On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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