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