Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

(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