Message-ID: <5B362B38.2000103@lab.ntt.co.jp>
Date: Fri, 29 Jun 2018 21:51:04 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with
 partition
 wise join enabled.
References: 
 <CAKcux6ktu-8tefLWtQuuZBYFaZA83vUzuRd7c1YHC-yEWyYFpg@mail.gmail.com>
 <5AE84FBF.3040905@lab.ntt.co.jp>
 <CAFjFpRdSvXxOkSMc=jEHfTVGnCFgziLA7m+onp=ak=Tfa_V1zA@mail.gmail.com>
 <5AF2E09F.9060208@lab.ntt.co.jp>
 <CAFjFpRcFucgvrzEFNi10g00-va640poSmJDmBrZNfFarB6eP-Q@mail.gmail.com>
 <5AF440C2.3040805@lab.ntt.co.jp>
 <CAFjFpRczEuG9b0_j_qve2JAi-HHqS9_m1_yK5QVQa5=oDr5mJA@mail.gmail.com>
 <5AF59415.10309@lab.ntt.co.jp>
 <CAFjFpRfkafBFJSEEkGwzLf=-y9Q8TUEGfpR=+K7-zCOw80R10w@mail.gmail.com>
 <CAFjFpRc8ZoDm0+zhx+MckwGyEqkOzWcpVqbvjaxwdGarZSNrmA@mail.gmail.com>
 <5AFC0865.8050802@lab.ntt.co.jp>
 <CAFjFpRfGL9w-4C5N-ioaG3m+iheC1ey7_RMZ4vJijGn=wWJyxw@mail.gmail.com>
 <5AFD6580.5090308@lab.ntt.co.jp> <5AFE81CC.8000508@lab.ntt.co.jp>
 <5B17C5EC.1090602@lab.ntt.co.jp>
 <CAFjFpRffA-BfL0RXcUVLLbcOBF1ie7oObTB-RE2q_o45X1HTYg@mail.gmail.com>
 <5B1E5071.1000704@lab.ntt.co.jp> <5B23A964.9020800@lab.ntt.co.jp>
 <5B28FB19.3090809@lab.ntt.co.jp>
 <CAFjFpRedQBySj5Vm-eq+gATP6sqJJfXzPrV+Ld+K6DnaOGkFAQ@mail.gmail.c
 om>
In-Reply-To: 
 <CAFjFpRedQBySj5Vm-eq+gATP6sqJJfXzPrV+Ld+K6DnaOGkFAQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
To: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Precedence: bulk

(2018/06/22 22:54), Ashutosh Bapat wrote:
> I have started reviewing the patch.

Thanks for the review!

> +       if (enable_partitionwise_join&&  rel->top_parent_is_partitioned)
> +       {
> +           build_childrel_tlist(root, rel, childrel, 1,&appinfo);
> +       }
>
> Why do we need rel->top_parent_is_partitioned? If a relation is
> partitioned (if (rel->part_scheme), it's either the top parent or is
> partition of some other partitioned table. In either case this
> condition will be true.

This would be needed to avoid unnecessarily applying 
build_childrel_tlist to child rels of a partitioned table for which we 
don't consider partitionwise join.  Consider:

postgres=# create table lpt (c1 int, c2 text) partition by list (c1);
CREATE TABLE
postgres=# create table lpt_p1 partition of lpt for values in (1);
CREATE TABLE
postgres=# create table lpt_p2 (c1 int check (c1 in (2)), c2 text);
CREATE TABLE
postgres=# create table test (c1 int, c2 text);
CREATE TABLE
postgres=# explain verbose select * from (select * from lpt union all 
select * from lpt_p2) ss(c1, c2) inner join test on (ss.c1 = test.c1);
                                      QUERY PLAN

--------------------------------------------------------------------------------
----
  Merge Join  (cost=289.92..538.20 rows=16129 width=72)
    Output: lpt_p1.c1, lpt_p1.c2, test.c1, test.c2
    Merge Cond: (test.c1 = lpt_p1.c1)
    ->  Sort  (cost=88.17..91.35 rows=1270 width=36)
          Output: test.c1, test.c2
          Sort Key: test.c1
          ->  Seq Scan on public.test  (cost=0.00..22.70 rows=1270 width=36)
                Output: test.c1, test.c2
    ->  Sort  (cost=201.74..208.09 rows=2540 width=36)
          Output: lpt_p1.c1, lpt_p1.c2
          Sort Key: lpt_p1.c1
          ->  Append  (cost=0.00..58.10 rows=2540 width=36)
                ->  Seq Scan on public.lpt_p1  (cost=0.00..22.70 
rows=1270 width=
36)
                      Output: lpt_p1.c1, lpt_p1.c2
                ->  Seq Scan on public.lpt_p2  (cost=0.00..22.70 
rows=1270 width=
36)
                      Output: lpt_p2.c1, lpt_p2.c2
(16 rows)

In this example, the top parent is not a partitioned table and we don't 
need to consider partitionwise join for that, so we don't need to apply 
that to the child rel lpt_p1 of the partitioned table lpt (and the table 
lpt_p2).

> +   /* No work if the child plan is an Append or MergeAppend */
> +   if (IsA(subplan, Append) || IsA(subplan, MergeAppend))
> +       return;
>
> Why? Probably it's related to the answer to the first question, But I
> don't see the connection. Given that partition-wise join will be
> applicable level by level, we need to recurse in
> adjust_subplan_tlist().

I don't think so; I think if the child plan is an Append or MergeAppend, 
the tlist for each subplan of the Append or MergeAppend would have 
already been adjusted while create_plan_recurse before we are called here.

> +   /* The child plan should be able to do projection */
> +   Assert(is_projection_capable_plan(subplan));
> +
> Again why? A MergeAppend's subplan could be a Sort plan, which will
> not be projection capable.

IIUC, since we are called here right after create_plan_recurse, the 
child plan would be a scan or join unless it's neither Append nor 
MergeAppend.  So it should be projection-capable.  Maybe I'm missing 
something, though.

> This is not a full review. I will continue reviewing it further.

Thanks again.

Best regards,
Etsuro Fujita