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

From: Rajkumar Raghuwanshi <[email-masked]>
Date: Mon, 12 Feb 2018 18:00:24 +0530

Hi,

I am getting "ERROR:  unexpected expression in subquery output" and
"ERROR:  variable not found in subplan target lists" errors, for "FOR
UPDATE" with postgres_fdw. (when set enable_partition_wise_join to true);

Attached patch have queries which are throwing mentioned error on running
make check in contrib/postgres_fdw folder.

An independent test case to reproduce this is given below.

SET enable_partition_wise_join TO true;

CREATE EXTENSION postgres_fdw;
CREATE SERVER fdw_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (dbname
'postgres', port '5432', use_remote_estimate 'true');
CREATE USER MAPPING FOR PUBLIC SERVER fdw_server;

CREATE TABLE pt1 ( c1 int NOT NULL, c2 int NOT NULL, c3 text)PARTITION BY
RANGE(c1);
CREATE TABLE pt1p1 (like pt1);
CREATE TABLE pt1p2 (like pt1);

CREATE TABLE pt2 (c1 int NOT NULL, c2 int NOT NULL, c3 text) PARTITION BY
RANGE(c1);
CREATE TABLE pt2p1 (like pt2);
CREATE TABLE pt2p2 (like pt2);

INSERT INTO pt1p1 SELECT id, id + 1,'AAA' || to_char(id, 'FM000') FROM
generate_series(-100, -1) id;
INSERT INTO pt1p2 SELECT id, id + 1,'AAA' || to_char(id, 'FM000') FROM
generate_series(1, 99) id;
INSERT INTO pt2p1 SELECT id, id + 1,'AAA' || to_char(id, 'FM000') FROM
generate_series(-100, -1) id;
INSERT INTO pt2p2 SELECT id, id + 1,'AAA' || to_char(id, 'FM000') FROM
generate_series(1, 99) id;

CREATE FOREIGN TABLE ft1p1 PARTITION OF pt1 FOR VALUES FROM (MINVALUE) TO
(0) SERVER fdw_server OPTIONS (table_name 'pt1p1');
CREATE FOREIGN TABLE ft1p2 PARTITION OF pt1 FOR VALUES FROM (0) TO
(MAXVALUE) SERVER fdw_server OPTIONS (table_name 'pt1p2');
CREATE FOREIGN TABLE ft2p1 PARTITION OF pt2 FOR VALUES FROM (MINVALUE) TO
(0) SERVER fdw_server OPTIONS (table_name 'pt2p1');
CREATE FOREIGN TABLE ft2p2 PARTITION OF pt2 FOR VALUES FROM (0) TO
(MAXVALUE) SERVER fdw_server OPTIONS (table_name 'pt2p2');

ANALYZE pt1;
ANALYZE pt2;
ANALYZE ft1p1;
ANALYZE ft1p2;
ANALYZE ft2p1;
ANALYZE ft2p2;

EXPLAIN (VERBOSE, COSTS OFF)
SELECT t1.c1, ss.a, ss.b FROM (SELECT c1 FROM pt1  WHERE c1 = 50) t1 INNER
JOIN (SELECT t2.c1, t3.c1 FROM (SELECT c1 FROM pt1  WHERE c1 between 50 and
60) t2 FULL JOIN (SELECT c1 FROM pt2 WHERE c1 between 50 and 60) t3 ON
(t2.c1 = t3.c1) WHERE t2.c1 IS NULL OR t2.c1 IS NOT NULL) ss(a, b) ON
(TRUE) ORDER BY t1.c1, ss.a, ss.b FOR UPDATE OF t1;
ERROR:  unexpected expression in subquery output

EXPLAIN (VERBOSE, COSTS OFF)
SELECT t1.c1, ss.a, ss.b FROM (SELECT c1 FROM pt1) t1 INNER JOIN (SELECT
t2.c1, t3.c1 FROM (SELECT c1 FROM pt1) t2 FULL JOIN (SELECT c1 FROM pt1) t3
ON (t2.c1 = t3.c1)) ss(a, b) ON (TRUE) ORDER BY t1.c1, ss.a, ss.b FOR
UPDATE OF t1;
ERROR:  variable not found in subplan target lists

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 68fdfdc..5466007 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1913,4 +1913,9 @@ EXPLAIN (COSTS OFF)
 SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 WHERE t1.a = t2.b AND t1.b = t2.a) q WHERE t1.a%25 = 0 ORDER BY 1,2;
 SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 WHERE t1.a = t2.b AND t1.b = t2.a) q WHERE t1.a%25 = 0 ORDER BY 1,2;
 
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT t1.a, ss.c1, ss.c2 FROM (SELECT a FROM fprt1) t1 INNER JOIN (SELECT t2.a, t3.a FROM (SELECT a FROM fprt1  WHERE a between 50 and 60) t2 FULL JOIN (SELECT a FROM fprt1) t3 ON (t2.a = t3.a)) ss(c1, c2) ON (TRUE) ORDER BY t1.a, ss.c1, ss.c2 FOR UPDATE OF t1;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT t1.a, ss.c1, ss.c2 FROM (SELECT a FROM fprt1) t1 INNER JOIN (SELECT t2.a, t3.a FROM (SELECT a FROM fprt1) t2 FULL JOIN (SELECT a FROM fprt1) t3 ON (t2.a = t3.a)) ss(c1, c2) ON (TRUE) ORDER BY t1.a, ss.c1, ss.c2 FOR UPDATE OF t1;
 RESET enable_partition_wise_join;

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 13 Feb 2018 18:21:04 +0530

On Mon, Feb 12, 2018 at 6:00 PM, Rajkumar Raghuwanshi
<[email-masked]> wrote:
> Hi,
>
> I am getting "ERROR:  unexpected expression in subquery output" and "ERROR:
> variable not found in subplan target lists" errors, for "FOR UPDATE" with
> postgres_fdw. (when set enable_partition_wise_join to true);
>

Thanks a lot for the patch. I can apply it and reproduce the error.

Here's my analysis of the bug.

The node for which this error comes is a ConvertRowtypeExpr node with
Var::varattno = 0 under it. Whole row reference of the parent is converted to
ConvertRowtypeExpr with whole row of child as an argument. When partition-wise
join is used, targetlist of child-joins contain such ConvertRowtypeExpr when
the parent-join's targetlist has whole-row references of joining
partitioned tables.

When we deparse the targetlist of join pushed down by postgres FDW,
build_tlist_to_deparse() pulls only Var nodes nodes from the join's targetlist.
So it pulls Var reprensenting a whole-row reference of a child from a
ConvertRowtypeExpr, when building targetlist to be deparsed for a child-join
with whole-row references. This targetlist is then saved as fdw_scan_tlist in
ForeignScanPlan.

This causes two problems shown by the two queries below

> (t2.c1 = t3.c1) WHERE t2.c1 IS NULL OR t2.c1 IS NOT NULL) ss(a, b) ON (TRUE)
> ORDER BY t1.c1, ss.a, ss.b FOR UPDATE OF t1;

get_relation_column_alias_ids() uses foreignrel's targetlist
(foreignrel->reltarget->exprs) as it is to locate given node to be deparsed.
If the joining relation corresponding to ConvertRowtypeExpr is deparsed as a
subquery, this function is called with whole-row reference node (Var node with
varattno = 0).  But the relation's targetlist doesn't contain its whole-row
reference directly but has it embedded in ConvertRowtypeExpr. So, the function
doesn't find the given node and throws error.

> ON (t2.c1 = t3.c1)) ss(a, b) ON (TRUE) ORDER BY t1.c1, ss.a, ss.b FOR UPDATE
> OF t1;

When there is possibility of EvalPlanQual being called, we construct local
join plan matching the pushed down foreign join. In postgresGetForeignPlan()
after we have built the local join plan, the topmost plan node's targetlist is
changed to fdw_scan_tlist to match the output of the ForeignScan node. As
explained above, this targetlist contains a bare reference to whole-row
reference of a child relation if the child-join's targetlist contains a
ConvertRowtypeExpr. When changing the topmost plan node's targetlist, we do not
modify the targetlists of its left and right tree nodes. The left/right plan
involving corresponding child relation will have ConvertRowtypeExpr expression
in its targetlist, but not whole-row reference directly. When the topmost local
join plan node's targetlist is processed by set_plan_ref(), it throws error
"variable not found in subplan target lists" since it doesn't find bare
whole-row reference of the child relation in subplan's targetlists.

The problem can be solved in two ways:

1. Push down ConvertRowtypeExpr and include it in the pushed down targetlist.
This would solve both the problems described above. Both set_plan_ref() and
get_relation_column_alias_ids() will find ConvertRowtypeExpr, they are looking
for and won't throw an error.

This requires two parts
a. In build_tlist_to_deparse(), instead of pulling
Var node from ConvertRowtypeExpr, we pull whole ConvertRowtypeExpr and include
it in the targetlist being deparsed which is also used as to set
fdw_scan_tlist. In order to pull any ConvertRowtypeExpr's in the local quals,
which may be hidden in the expression tree, we will add two more options to
flags viz. PVC_INCLUDE_CONVERTROWTYPEEXPR and PVC_RECURSE_CONVERTROWTYPEEXPR.
Unlike the other PVC_* options, which do not default to a value, we may want to
default to PVC_RECURSE_CONVERTROWTYPEEXPR if nothing is specified. That will
avoid, possibly updating every pull_var_clause call with
PVC_RECURSE_CONVERTROWTYPEEXPR.
b. deparse ConvertRowtypeExpr
For this we need to get the conversion map between the parent and child. We
then deparse ConvertRowtypeExpr as a ROW() with the attributes of child
rearranged per the conversion map. A multi-level partitioned table will have
nested ConvertRowtypeExpr. To deparse such expressions, we need to find the
conversion map between the topmost parent and the child, by ignoring any
intermediate parents.

2. Modify the local join plan entirely to contain whole row reference of
child relations instead of ConvertRowtypeExpr and deparse a ConvertRowtypeExpr
as a whole-row reference in a subquery.
Again we need two part solution:
a. For this we need to write a walker which walks the plan tree distributing the
Vars in the topmost targetlist to the left and right subtrees. Thus we replace
ConvertRowtypeExpr with corresponding whole-row references in the whole plan
tree.
b. When get_relation_column_alias_ids() encounters a
ConvertRowtypeExpr, it pulls
out the embedded whole row reference and returns the corresponding column id.
deparseExpr() calls deparseVar() by pulling out the embedded whole-row
reference Var when it encouters ConvertRowtypeExpr.

Comments, thoughts any other solutions?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 14 Feb 2018 18:20:27 +0900

> whole-row reference of the child relation in subplan's targetlists.

Thanks for the analysis!

> reference Var when it encouters ConvertRowtypeExpr.

I'd vote for #1, but ISTM that that's more like a feature, not a fix. 
Pushing down ConvertRowtypeExprs to the remote seems to me to be the 
same problem as pushing down PHVs to the remote, which is definitely a 
feature development.  I think a fix for this would be to just give up 
pushing down a child join in foreign_join_ok or somewhere else if the 
reltarget of the child-join relation contains any ConvertRowtypeExprs.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 14 Feb 2018 15:34:52 +0530

On Wed, Feb 14, 2018 at 2:50 PM, Etsuro Fujita
<[email-masked]> wrote:
> Pushing down ConvertRowtypeExprs to the remote seems to me to be the same
> problem as pushing down PHVs to the remote, which is definitely a feature
> development.  I think a fix for this would be to just give up pushing down a
> child join in foreign_join_ok or somewhere else if the reltarget of the
> child-join relation contains any ConvertRowtypeExprs.

That means we won't be able to push down any scans under a DML on a
partitioned table. That will be too restrictive.

I think the case of PHV and ConvertRowtypeExprs are different. The the
former we need a subquery to push PHVs, whereas to deparse
ConvertRowtypeExpr on the nullable side we can use the same strategy
as whole-row expression deparsing.

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 20 Feb 2018 14:43:02 +0530

> conversion map between the topmost parent and the child, by ignoring any

Here's patchset implementing this solution.

0001 adds PVC_*_CONVERTROWTYPEEXPR to pull_var_clause() and adjusts its callers.

0002 fixes a similar bug for regular partitioned tables. The patch has
testcase. The commit message explains the bug in more detail.

0003 has postgres_fdw fixes as outlined above with tests.

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 17 Apr 2018 17:35:44 +0900

> 0003 has postgres_fdw fixes as outlined above with tests.

The patch 0003 doesn't applies to the latest HEAD any more, so please 
update the patch.  I think this should be an open item for PG11.  My 
apologizes for not having reviewed the patch in the last commitfest.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 17 Apr 2018 15:13:51 +0530

On Tue, Apr 17, 2018 at 2:05 PM, Etsuro Fujita
<[email-masked]> wrote:
> The patch 0003 doesn't applies to the latest HEAD any more, so please update
> the patch.  I think this should be an open item for PG11.  My apologizes for
> not having reviewed the patch in the last commitfest.

Here's updated patch-set.

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 17 Apr 2018 19:00:01 +0900

> Here's updated patch-set.

Thanks for updating the patch set!  Will review.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 24 Apr 2018 20:19:45 +0900

(2018/04/17 19:00), Etsuro Fujita wrote:
> Will review.

I started reviewing this.

o 0001-Handle-ConvertRowtypeExprs-in-pull_vars_clause.patch:

+   else if (IsA(node, ConvertRowtypeExpr))
+   {
+#ifdef USE_ASSERT_CHECKING
+       ConvertRowtypeExpr *cre = castNode(ConvertRowtypeExpr, node);
+       Var        *var;
+
+       /*
+        * ConvertRowtypeExprs only result when parent's whole-row 
reference is
+        * translated for a child using adjust_appendrel_attrs(). That 
function
+        * does not handle any upper level Var references.
+        */
+       while (IsA(cre->arg, ConvertRowtypeExpr))
+           cre = castNode(ConvertRowtypeExpr, cre->arg);
+       var = castNode(Var, cre->arg);
+       Assert (var->varlevelsup == 0);
+#endif /* USE_ASSERT_CHECKING */

Isn't it better to throw ERROR as in other cases in pull_vars_clause()? 
  Another thing I noticed about this patch is this:

postgres=# create table prt1 (a int, b int, c varchar) partition by 
range (a);
postgres=# create table prt1_p1 partition of prt1 FOR VALUES FROM (0) TO 
(250);
postgres=# create table prt1_p2 partition of prt1 FOR VALUES FROM (250) 
TO (500)
;
postgres=# insert into prt1 select i, i % 25, to_char(i, 'FM0000') from 
generate
_series(0, 499) i where i % 2 = 0;
postgres=# analyze prt1;
postgres=# create table prt2 (a int, b int, c varchar) partition by 
range (b);
postgres=# create table prt2_p1 partition of prt2 FOR VALUES FROM (0) TO 
(250);
postgres=# create table prt2_p2 partition of prt2 FOR VALUES FROM (250) 
TO (500)
;
postgres=# insert into prt2 select i % 25, i, to_char(i, 'FM0000') from 
generate
_series(0, 499) i where i % 3 = 0;
postgres=# analyze prt2;

postgres=# update prt1 t1 set c = 'foo' from prt2 t2 where t1::text = 
t2::text a
nd t1.a = t2.b;
ERROR:  ConvertRowtypeExpr found where not expected

To fix this, I think we need to pass PVC_RECURSE_CONVERTROWTYPES to 
pull_vars_clause() in distribute_qual_to_rels() and 
generate_base_implied_equalities_no_const() as well.

That's all I have for now.  Will continue the review.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 25 Apr 2018 15:21:48 +0530

On Tue, Apr 24, 2018 at 4:49 PM, Etsuro Fujita
<[email-masked]> wrote:
> +        * ConvertRowtypeExprs only result when parent's whole-row reference
> is

Actually I noticed that ConvertRowtypeExpr are used to cast a child's
whole row reference expression (not just a Var node) into that of its
parent and not. For example a cast like NULL::child::parent produces a
ConvertRowtypeExpr encapsulating a NULL constant node and not a Var
node. We are interested only in ConvertRowtypeExprs embedding Var
nodes with Var::varattno = 0. I have changed this code to use function
is_converted_whole_row_reference() instead of the above code with
Assert. In order to use that function, I had to take it out of
setrefs.c and put it in nodeFuncs.c which seemed to be a better fit.

> postgres=# create table prt1 (a int, b int, c varchar) partition by range
> (a);
> postgres=# create table prt1_p2 partition of prt1 FOR VALUES FROM (250) TO
> (500)
> ;
> postgres=# create table prt2 (a int, b int, c varchar) partition by range
> (b);
> postgres=# create table prt2_p2 partition of prt2 FOR VALUES FROM (250) TO
> (500)
> ;

Thanks for the catch. I have updated patch with your suggested fix.

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 26 Apr 2018 21:06:43 +0900

> Thanks for the catch. I have updated patch with your suggested fix.

Thanks, but I don't think that's enough.  Consider:

postgres=# create table base_tbl (a int, b int);
postgres=# insert into base_tbl select i % 25, i from generate_series(0, 
499) i where i % 6 = 0;

postgres=# update prt1 t1 set c = 'foo' from prt2 t2 left join (select 
a, b, 1 from base_tbl) ss(c1, c2, c3) on (t2.b = ss.c2) where t1::text = 
t2::text and t1.a = t2.b;
ERROR:  ConvertRowtypeExpr found where not expected

To fix this, I think we need to pass PVC_RECURSE_CONVERTROWTYPES to 
pull_var_clause() in find_placeholders_in_expr() as well.

The patch doesn't pass that to pull_var_clause() in other places such as 
fix_placeholder_input_needed_levels() or planner.c, but I don't 
understand the reason why that's OK.

Sorry, I've not finished the review yet, so I'll continue that.

Thanks for updating the patch!

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 27 Apr 2018 11:10:45 +0530

On Thu, Apr 26, 2018 at 5:36 PM, Etsuro Fujita
<[email-masked]> wrote:
>>> TO
>>> TO
> postgres=# update prt1 t1 set c = 'foo' from prt2 t2 left join (select a, b,
> 1 from base_tbl) ss(c1, c2, c3) on (t2.b = ss.c2) where t1::text = t2::text
> and t1.a = t2.b;

Thanks for the test.

> fix_placeholder_input_needed_levels() or planner.c, but I don't understand
> the reason why that's OK.

I was trying to be conservative not to include
PVC_RECURSE_CONVERTROWTYPES everywhere. But it looks like because of
inheritance_planner() and partition-wise aggregate commit, we can have
ConvertRowtypeExprs almost everywhere. Added
PVC_RECURSE_CONVERTROWTYPEs in almost all the places except the ones
listed in 0001 patch.

> Sorry, I've not finished the review yet, so I'll continue that.

Thanks for the tests and review.

Here's updated patch set.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 01 May 2018 20:30:07 +0900

> Here's updated patch set.

Thanks for updating the patch!  Here are my review comments on patch 
0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:

* This assertion in deparseConvertRowtypeExpr wouldn't always be true 
because of cases like ALTER FOREIGN TABLE DROP COLUMN plus ALTER FOREIGN 
TABLE ADD COLUMN:

+   Assert(parent_desc->natts == child_desc->natts);

Here is such an example:

postgres=# create table fprt1 (a int, b int, c varchar) partition by 
range (a);
postgres=# create table fprt1_p1 (like fprt1);
postgres=# create table fprt1_p2 (like fprt1);
postgres=# create foreign table ftprt1_p1 (a int, b int, c varchar) 
server loopback options (table_name 'fprt1_p1', use_remote_estimate 'true');
postgres=# alter foreign table ftprt1_p1 drop column c;
postgres=# alter foreign table ftprt1_p1 add column c varchar;
postgres=# alter table fprt1 attach partition ftprt1_p1 for values from 
(0) to (250);
postgres=# create foreign table ftprt1_p2 partition of fprt1 for values 
from (250) to (500) server loopback options (table_name 'fprt1_p2', 
use_remote_estimate 'true');
postgres=# insert into fprt1 select i, i, to_char(i/50, 'FM0000') from 
generate_series(0, 499, 2) i;
postgres=# analyze fprt1;
postgres=# analyze fprt1_p1;
postgres=# analyze fprt1_p2;
postgres=# create table fprt2 (a int, b int, c varchar) partition by 
range (b);
postgres=# create table fprt2_p1 (like fprt2);
postgres=# create table fprt2_p2 (like fprt2);
postgres=# create foreign table ftprt2_p1 partition of fprt2 for values 
from (0) to (250) server loopback options (table_name 'fprt2_p1', 
use_remote_estimate 'true');
postgres=# create foreign table ftprt2_p2 partition of fprt2 for values 
from (250) to (500) server loopback options (table_name 'fprt2_p2', 
use_remote_estimate 'true');
postgres=# insert into fprt2 select i, i, to_char(i/50, 'FM0000') from 
generate_series(0, 499, 3) i;
postgres=# analyze fprt2;
postgres=# analyze fprt2_p1;
postgres=# analyze fprt2_p2;
postgres=# set enable_partitionwise_join to true;
postgres=# select t1, t2 from fprt1 t1 join fprt2 t2 on (t1.a = t2.b) 
where t1.a % 25 = 0;
server closed the connection unexpectedly
         This probably means the server terminated abnormally
         before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

I think we should remove that assertion.

* But I don't think that change is enough.  Here is another oddity with 
that assertion removed.

postgres=# alter table fprt1 detach partition ftprt1_p1;
postgres=# alter table fprt1 detach partition ftprt1_p2;
postgres=# alter table fprt1 drop column c;
postgres=# alter table fprt1 add column c varchar;
postgres=# alter table fprt1 attach partition ftprt1_p1 for values from 
(0) to (250);
postgres=# alter table fprt1 attach partition ftprt1_p2 for values from 
(250) to (500);
postgres=# set enable_partitionwise_join to true;
postgres=# select t1, t2 from fprt1 t1 join fprt2 t2 on (t1.a = t2.b) 
where t1.a % 25 = 0;
ERROR:  malformed record literal: "(300,300,"(300,300,0006)",0006)"
DETAIL:  Too many columns.
CONTEXT:  processing expression at position 1 in select list

The reason for that would be: in the following, the patch doesn't take 
care of dropped columns in the parent table (ie, columns such that 
attrMap[cnt]=0).

+   /* Construct ROW expression according to the conversion map. */
+   appendStringInfoString(buf, "ROW(");
+   for (cnt = 0; cnt < natts; cnt++)
+   {
+       if (cnt > 0)
+           appendStringInfoString(buf, ", ");
+       deparseColumnRef(buf, child_var->varno, attrMap[cnt], root,
+                        qualify_col);
+   }
+   appendStringInfoChar(buf, ')');

To fix this, I think we should skip the deparseColumnRef processing for 
dropped columns in the parent table.

* I think it would be better to do EXPLAIN with the VERBOSE option to 
the queries this patch added to the regression tests, to see if 
ConvertRowtypeExprs are correctly deparsed in the remote queries.

That's all I have for now.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 8 May 2018 12:50:48 +0530

> I think we should remove that assertion.

Removed.

>
> * But I don't think that change is enough.  Here is another oddity with that
> assertion removed.

> dropped columns in the parent table.

Done.

I have changed the test file a bit to test these scenarios.

Thanks for testing.

>
> * I think it would be better to do EXPLAIN with the VERBOSE option to the
> queries this patch added to the regression tests, to see if

The reason VERBOSE option was omitted for partition-wise join was to
avoid repeated and excessive output for every child-join. I think that
still applies.

PFA patch-set with the fixes.

I also noticed that the new function deparseConvertRowtypeExpr is not
quite following the naming convention of the other deparseFoo
functions. Foo is usually the type of the node the parser would
produced when the SQL string produced by that function is parsed. That
doesn't hold for the SQL string produced by ConvertRowtypeExpr but
then naming it as generic deparseRowExpr() wouldn't be correct either.
And then there are functions like deparseColumnRef which may produce a
variety of SQL strings which get parsed into different node types e.g.
a whole-row reference would produce ROW(...) which gets parsed as a
RowExpr. Please let me know if you have any suggestions for the name.

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 09 May 2018 20:50:55 +0900

(2018/05/08 16:20), Ashutosh Bapat wrote:
>> Here are my review comments on patch

>> dropped columns in the parent table.

Thanks for those changes!

> I have changed the test file a bit to test these scenarios.

+1

> avoid repeated and excessive output for every child-join. I think that

I'd like to leave that for the committer's judge.

> RowExpr. Please let me know if you have any suggestions for the name.

To be honest, I don't have any strong opinion about that.  But I like 
"deparseConvertRowtypeExpr" because that name seems to well represent 
the functionality, so I'd vote for that.

BTW, one thing I'm a bit concerned about is this:

 > setrefs.c and put it in nodeFuncs.c which seemed to be a better fit.

This change seems a bit confusing to me because the flag bits 
"PVC_INCLUDE_CONVERTROWTYPES" and "PVC_RECURSE_CONVERTROWTYPES" passed 
to pull_var_clause look as if it handles any ConvertRowtypeExpr nodes 
from a given clause, but really, it only handles ConvertRowtypeExprs you 
mentioned above.  To make that function easy to understand and use, I 
think it'd be better to use the IsA(node, ConvertRowtypeExpr) test as in 
the first version of the patch, instead of 
is_converted_whole_row_reference, which would be more consistent with 
other cases such as PlaceHolderVar.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 10 May 2018 09:34:33 +0530

On Wed, May 9, 2018 at 5:20 PM, Etsuro Fujita
<[email-masked]> wrote:
> "PVC_INCLUDE_CONVERTROWTYPES" and "PVC_RECURSE_CONVERTROWTYPES" passed to
> pull_var_clause look as if it handles any ConvertRowtypeExpr nodes from a
> given clause, but really, it only handles ConvertRowtypeExprs you mentioned
> above.  To make that function easy to understand and use, I think it'd be
> better to use the IsA(node, ConvertRowtypeExpr) test as in the first version
> of the patch, instead of is_converted_whole_row_reference, which would be
> more consistent with other cases such as PlaceHolderVar.

I agree that using is_converted_whole_row_reference() is not
consistent with the other nodes that are handled by pull_var_clause().
I also agree that PVC_*_CONVERTROWTYPES doesn't reflect exactly what's
being done with those options. But using
is_converted_whole_row_reference() is the correct thing to do since we
are interested only in the whole-row references embedded in
ConvertRowtypeExpr. There can be anything encapsulated in
ConvertRowtypeExpr(), a non-shippable function for example. We don't
want to try to push that down in postgres_fdw's case. Neither in other
cases. So, it boils down to changing names of PVC_*_CONVERTROWTYPES to
something more meaningful. How about PVC_*_CONVERTWHOLEROWS?

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 10 May 2018 21:53:22 +0900

(2018/05/10 13:04), Ashutosh Bapat wrote:
> cases.

Yeah, but I think the IsA test would be sufficient to make things work 
correctly because we can assume that there aren't any other nodes than 
Var, PHV, and CRE translating a wholerow value of a child table into a 
wholerow value of its parent table's rowtype in the expression clause to 
which we apply pull_var_clause with PVC_INCLUDE_CONVERTROWTYPES.

BTW, one thing I noticed about the new version of the patch is: there is 
yet another case where pull_var_clause produces an error.  Here is an 
example:

postgres=# create table t1 (a int, b text);
CREATE TABLE
postgres=# create table t2 (a int, b text);
CREATE TABLE
postgres=# create foreign table ft1 (a int, b text) server loopback 
options (table_name 't1');
CREATE FOREIGN TABLE
postgres=# create foreign table ft2 (a int, b text) server loopback 
options (table_name 't2');
CREATE FOREIGN TABLE
postgres=# insert into ft1 values (1, 'foo');
INSERT 0 1
postgres=# insert into ft1 values (2, 'bar');
INSERT 0 1
postgres=# insert into ft2 values (1, 'test1');
INSERT 0 1
postgres=# insert into ft2 values (2, 'test2');
INSERT 0 1
postgres=# analyze ft1;
ANALYZE
postgres=# analyze ft2;
ANALYZE
postgres=# create table parent (a int, b text);
CREATE TABLE
postgres=# alter foreign table ft1 inherit parent;
ALTER FOREIGN TABLE
postgres=# explain verbose update parent set b = ft2.b from ft2 where 
parent.a = ft2.a returning parent;
ERROR:  ConvertRowtypeExpr found where not expected

To produce this, apply the patch in [1] as well as the new version; 
without that patch in [1], the update query would crash the server (see 
[1] for details).  To fix this, I think we need to pass 
PVC_RECURSE_CONVERTROWTYPES to pull_var_clause in build_remote_returning 
in postgres_fdw.c as well.

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5AF43E02.30000%40lab.ntt.co.jp

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 11 May 2018 12:47:55 +0530

On Thu, May 10, 2018 at 6:23 PM, Etsuro Fujita
<[email-masked]> wrote:
> correctly because we can assume that there aren't any other nodes than Var,
> PHV, and CRE translating a wholerow value of a child table into a wholerow
> value of its parent table's rowtype in the expression clause to which we
> apply pull_var_clause with PVC_INCLUDE_CONVERTROWTYPES.

Not really.
Consider following case using the same tables fprt1 and fprt2 in test
sql/postgres_fdw.sql
create function row_as_is(a ftprt2_p1) returns ftprt2_p1 as $$begin
return a; end;$$ language plpgsql;

With the change you propose i.e.

diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c
index f972712..088da14 100644
--- a/src/backend/optimizer/util/var.c
+++ b/src/backend/optimizer/util/var.c
@@ -646,8 +646,9 @@ pull_var_clause_walker(Node *node,
pull_var_clause_context *context)
                else
                        elog(ERROR, "PlaceHolderVar found where not expected");
        }
-       else if (is_converted_whole_row_reference(node))
+       else if (IsA(node, ConvertRowtypeExpr))
        {
+               Assert(is_converted_whole_row_reference(node));
                if (context->flags & PVC_INCLUDE_CONVERTROWTYPES)
                {
                        context->varlist = lappend(context->varlist, node);

following query crashes at Assert(is_converted_whole_row_reference(node));

If we remove that assert, it would end up pushing down row_as_is(),
which is a local user defined function, to the foreign server. That's
not desirable since the foreign server may not have that user defined
function.

>
> BTW, one thing I noticed about the new version of the patch is: there is yet
> another case where pull_var_clause produces an error.  Here is an example:
>
> postgres=# create foreign table ft1 (a int, b text) server loopback options
> (table_name 't1');
> CREATE FOREIGN TABLE
> postgres=# create foreign table ft2 (a int, b text) server loopback options
> (table_name 't2');
> CREATE FOREIGN TABLE
> To produce this, apply the patch in [1] as well as the new version; without
> that patch in [1], the update query would crash the server (see [1] for
> details).  To fix this, I think we need to pass PVC_RECURSE_CONVERTROWTYPES
> to pull_var_clause in build_remote_returning in postgres_fdw.c as well.

I missed that call to PVC since it was added after I had created my
patches. That's a disadvantage of having changed PVC to use flags
instead of bools. We won't notice such changes. Thanks for pointing it
out. Changed as per your suggestion.

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 11 May 2018 22:01:09 +0900

(2018/05/11 16:17), Ashutosh Bapat wrote:
>                  if (context->flags&  PVC_INCLUDE_CONVERTROWTYPES)
>                  {

I guess you forgot to show the query.

> not desirable since the foreign server may not have that user defined

I don't understand the counter-example you tried to show, but I think 
that I was wrong here; the IsA test isn't sufficient.

> out. Changed as per your suggestion.

Thanks for that change and the updated version!

Yet yet another case where pull_var_clause produces an error:

postgres=# create table list_pt (a int, b text) partition by list (a);
CREATE TABLE
postgres=# create table list_ptp1 partition of list_pt for values in (1);
CREATE TABLE
postgres=# alter table list_ptp1 add constraint list_ptp1_check check 
(list_ptp1::list_pt::text != NULL);
ERROR:  ConvertRowtypeExpr found where not expected

The patch kept the flags passed to pull_var_clause in 
src/backend/catalog/heap.c as-is, but I think we need to modify that 
accordingly.  Probably, the flags passed to pull_var_clause in 
CreateTrigger as well.

Having said that, I started to think this approach would make code much 
complicated.  Another idea I came up with to avoid that would be to use 
pull_var_clause as-is and then rewrite wholerows of partitions back to 
ConvertRowtypeExprs translating those wholerows to wholerows of their 
parents tables' rowtypes if necessary.  That would be annoying and a 
little bit inefficient, but the places where we need to rewrite is 
limited: prepare_sort_from_pathkeys and build_tlist_to_deparse, IIUC. 
So we could really minimize the code change and avoid the additional 
overhead caused by the is_converted_whole_row_reference test added to 
pull_var_clause.  I think the latter would be rather important, because 
PWJ is disabled by default.  What do you think about that approach?

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 14 May 2018 10:21:25 +0530

> I guess you forgot to show the query.

Sorry. Here's the query.

explain verbose select * from fprt1 t1 join fprt2 t2 on t1.a = t2.b
where t1 = row_as_is(row(t2.a, t2.b, t2.c)::ftprt2_p1)::fprt2;

> accordingly.  Probably, the flags passed to pull_var_clause in CreateTrigger
> as well.

With all the support that we have added in partitioning for v11, I
think we need to add PVC_RECURSE_CONVERTROWTYPE at all the places,
which were left unchanged in the earlier versions of patches, which
were written when all that support wasn't in v11.

> parents tables' rowtypes if necessary.  That would be annoying and a little
> bit inefficient, but the places where we need to rewrite is limited:
> prepare_sort_from_pathkeys and build_tlist_to_deparse, IIUC.

Other reason why we can't use your approach is we can not decide
whether ConvertRowtypeExpr is needed by just looking at the Var node.
You might say, that we add a ConvertRowtypeExpr if the Var::varno
references a child relation. But that's not true. For example a whole
row reference embedded in ConvertRowtypeExpr added by query
adjustments in inheritance_planner() is not a child's whole-row
reference in the adjusted query. So, it's not clear to me when we add
a ConvertRowtypeExpr and we don't. I am not sure if those are the only
two places which require a fix. Right now it looks like those are the
places which need PVC_INCLUDE_CONVERTROWTYPE, but that may not be true
in the future, esp. given the amount of work we expect to happen in
the partitioning area. When that happens, fixing all those places in
that way wouldn't be good.

> So we could
> really minimize the code change and avoid the additional overhead caused by
> the is_converted_whole_row_reference test added to pull_var_clause.  I think
> the latter would be rather important, because PWJ is disabled by default.
> What do you think about that approach?

Partition-wise join and partition-wise aggregation are disabled by
default temporarily. We will change it in near future once the memory
consumption issue has been tackled. The features are useful and users
would want those to be enabled by default like other query
optimizations. So, we can't take a decision based on that.

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 14 May 2018 12:04:04 +0530

> optimizations. So, we can't take a decision based on that.

Here's set of updated patches.

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 16 May 2018 19:31:01 +0900

(2018/05/14 15:34), Ashutosh Bapat wrote:
>> Here's the query.

Thanks!

>> were written when all that support wasn't in v11.

Actually, we'd get the same error without using anything in 
partitioning.  Here is an example:

postgres=# create table parent (a int, b text);
CREATE TABLE
postgres=# create table child (a int, b text);
CREATE TABLE
postgres=# alter table child inherit parent ;
ALTER TABLE
postgres=# alter table child add constraint child_check check 
(child::parent::text != NULL);
ERROR:  ConvertRowtypeExpr found where not expected

>> reference in the adjusted query.

Sorry, I don't understand this fully.

>> So, it's not clear to me when we add
>> a ConvertRowtypeExpr and we don't.

I think it'd be OK to rewrite so at least in prepare_sort_from_pathkeys 
and build_tlist_to_deparse.

>> I am not sure if those are the only

I have to admit that the approach I proposed is a band-aid fix.

>> optimizations. So, we can't take a decision based on that.

Yeah, I really agree on that point!  However, considering that 
pull_var_clause is used in many places where partitioning is *not* 
involved, I still think it's better to avoid spending extra cycles 
needed only for partitioning in that function.

Here are some other minor comments on patch 
0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:

+   /* Construct the conversion map. */
+   parent_desc = lookup_rowtype_tupdesc(cre->resulttype, 0);

I think it'd be better to pass -1, not 0, as the typmod argument for 
that function because that would be consistent with other places where 
we use that function with named rowtypes (eg, ruleutils.c).

-is_subquery_var(Var *node, RelOptInfo *foreignrel, int *relno, int *colno)
+is_subquery_column(Node *node, Index varno, RelOptInfo *foreignrel,
+               int *relno, int *colno)

-1 for that change; because 1) we use "var" for implying many things as 
in eg, pull_var_clause and 2) I think it'd make back-patching easy to 
keep the name as-is.

Other than that the patch set looks good to me.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 16 May 2018 19:19:05 +0530

On Wed, May 16, 2018 at 4:01 PM, Etsuro Fujita
<[email-masked]> wrote:
> pull_var_clause is used in many places where partitioning is *not* involved,
> I still think it's better to avoid spending extra cycles needed only for
> partitioning in that function.

Right. The changes done in inheritance_planner() imply that we can
encounter a ConvertRowtypeExpr even in the expressions for a relation
which is not a child relation in the translated query. It was a child
in the original query but after translating it becomes a parent and
has ConvertRowtypeExpr somewhere there. So, there is no way to know
whether a given expression will have ConvertRowtypeExpr in it. That's
my understanding. But if we can device such a test, I am happy to
apply that test before or witin the call to pull_var_clause().

We don't need that expensive test if user has passed
PVC_RECURSE_CONVERTROWTYPE. So we could test that first and then check
the shape of expression tree. It would cause more asymmetry in
pull_var_clause(), but we can live with it or change the order of
tests for all the three options. I will differ that to a committer.

> I think it'd be better to pass -1, not 0, as the typmod argument for that
> function because that would be consistent with other places where we use
> that function with named rowtypes (eg, ruleutils.c).

Done.

> -1 for that change; because 1) we use "var" for implying many things as in
> eg, pull_var_clause and 2) I think it'd make back-patching easy to keep the
> name as-is.

I think pull_var_clause is the only place where we do that and I don't
like that very much. I also don't like is_subquery_var() name since
it's too specific. We might want that kind of functionality to ship
generic expressions and not just Vars in future. Usually we would be
searching for columns in the subquery targetlist so the name change
looks good to me. IIRC, the function was added one release back, so
backpatching pain, if any, won't be much. But I don't think we should
carry a misnomer for many releases to come. Let's differ this to a
committer.

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 17 May 2018 20:20:32 +0900

(2018/05/16 22:49), Ashutosh Bapat wrote:
>> However, considering that
> has ConvertRowtypeExpr somewhere there.

Sorry, I don't understand this.  Could you show me such an example?

> So, there is no way to know

Sounds more invasive.  Considering where we are in the development cycle 
for PG11, I think it'd be better to address this by something like the 
approach I proposed.  Anyway, +1 for asking the committer's opinion.

Thanks!

> carry a misnomer for many releases to come. Let's differ this to a

OK

> Here's set of updated patches.

Thanks for updating the patch set!  I noticed followings.  Sorry for not 
having noticed that before.

- On patch 0001-Handle-ConvertRowtypeExprs-in-pull_vars_clause.patch:

+extern bool
+is_converted_whole_row_reference(Node *node)

I think we should remove "extern" from the definition.

- On patch 0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:

+   /* Construct ROW expression according to the conversion map. */
+   appendStringInfoString(buf, "ROW(");
+   for (cnt = 0; cnt < parent_desc->natts; cnt++)
+   {
+       /* Ignore dropped columns. */
+       if (attrMap[cnt] == 0)
+           continue;
+
+       if (cnt > 0)
+           appendStringInfoString(buf, ", ");
+       deparseColumnRef(buf, child_var->varno, attrMap[cnt],
+                        planner_rt_fetch(child_var->varno, root),
+                        qualify_col);
+   }
+   appendStringInfoChar(buf, ')');

The result would start with ", " in the case where the cnt=0 column of 
the parent relation is a dropped one.  Here is an example:

postgres=# create table pt1 (a int, b int) partition by range (b);
CREATE TABLE
postgres=# alter table pt1 drop column a;
ALTER TABLE
postgres=# alter table pt1 add column a int;
ALTER TABLE
postgres=# create table loct11 (like pt1);
CREATE TABLE
postgres=# create foreign table pt1p1 partition of pt1 for values from 
(0) to (100) server loopback options (table_name 'loct11', 
use_remote_estimate 'true');
CREATE FOREIGN TABLE
postgres=# insert into pt1 select i, i from generate_series(0, 99, 2) i;
INSERT 0 50
postgres=# analyze pt1;
ANALYZE
postgres=# analyze pt1p1;
ANALYZE
postgres=# create table pt2 (a int, b int) partition by range (b);
CREATE TABLE
postgres=# create table loct21 (like pt2);
CREATE TABLE
postgres=# create foreign table pt2p1 partition of pt2 for values from 
(0) to (100) server loopback options (table_name 'loct21', 
use_remote_estimate 'true');
CREATE FOREIGN TABLE
postgres=# insert into pt2 select i, i from generate_series(0, 99, 3) i;
INSERT 0 34
postgres=# analyze pt2;
ANALYZE
postgres=# analyze pt2p1;
ANALYZE
postgres=# set enable_partitionwise_join to true;
SET
postgres=# explain verbose select pt1.* from pt1, pt2 where pt1.b = 
pt2.b for update of pt1;
ERROR:  syntax error at or near ","
CONTEXT:  remote SQL command: EXPLAIN SELECT r4.b, r4.a, r4.ctid, CASE 
WHEN (r4.*)::text IS NOT NULL THEN ROW(, r4.b, r4.a) END, CASE WHEN 
(r4.*)::text IS NOT NULL THEN 57467 END, r6.ctid, CASE WHEN (r6.*)::text 
IS NOT NULL THEN ROW(r6.a, r6.b) END, CASE WHEN (r6.*)::text IS NOT NULL 
THEN 57476 END FROM (public.loct11 r4 INNER JOIN public.loct21 r6 ON 
(((r4.b = r6.b)))) FOR UPDATE OF r4

I think we can fix this by adding another flag to indicate whether we 
deparsed the first live column of the relation, as in the "first" bool 
flag in deparseTargetList.

One more thing to add is: the patch adds support for deparsing a 
ConvertRowtypeExpr that translates a wholerow of a childrel into a 
wholerow of its parentrel's rowtype, so by modifying is_foreign_expr 
accordingly, we could push down such CREs in JOIN ON conditions to the 
remote for example.  But that would be an improvement, not a fix, so I 
think we should leave that for another patch for PG12.

Other than that, the patch set looks good to me.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 17 May 2018 19:49:03 +0530

On Thu, May 17, 2018 at 4:50 PM, Etsuro Fujita
<[email-masked]> wrote:
>>> involved,

Even without inheritance we can induce a ConvertRowtypeExpr on a base
relation which is not "other" relation

create table parent (a int, b int);
create table child () inherits(parent);
alter table child add constraint whole_par_const check ((child::parent).a = 1);

There is no way to tell by looking at the parsed query whether
pull_var_clause() from StoreRelCheck() will encounter a
ConvertRowtypeExpr or not. We could check whether the tables involved
are inherited or not, but that's more expensive than
is_converted_whole_row_reference().

> Sounds more invasive.  Considering where we are in the development cycle for
> PG11, I think it'd be better to address this by something like the approach
> I proposed.  Anyway, +1 for asking the committer's opinion.

Thanks.

Done.

> deparsed the first live column of the relation, as in the "first" bool flag
> in deparseTargetList.

Thanks. Fixed.

> ConvertRowtypeExpr that translates a wholerow of a childrel into a wholerow
> of its parentrel's rowtype, so by modifying is_foreign_expr accordingly, we
> could push down such CREs in JOIN ON conditions to the remote for example.
> But that would be an improvement, not a fix, so I think we should leave that
> for another patch for PG12.

Right.

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 18 May 2018 16:33:32 +0900

> is_converted_whole_row_reference().

Yeah, ISTM that the inheritance test makes things worse.

>> I proposed.  Anyway, +1 for asking the committer's opinion.

Let's ask that.

>> deparsed the first live column of the relation, as in the "first" bool flag

Thanks for updating the patch set!  Other than pull_var_clause things, 
the updated version looks good to me, so I'll mark this as Ready for 
Committer.  As I said before, I think this issue should be addressed in 
advance of the PG11 release.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 06 Jun 2018 20:30:52 +0900

(2018/05/18 16:33), Etsuro Fujita wrote:
> Other than pull_var_clause things,
> Committer.

Since I'm not 100% sure that that is the right way to go, I've been 
rethinking how to fix this issue.  Yet another idea I came up with to 
fix this is to redesign the handling of the tlists for children in the 
partitioning case.  Currently, we build the reltarget for a child by 
applying adjust_appendrel_attrs to the reltarget for its parent in 
set_append_rel_size, which maps a wholerow Var referring to the parent 
rel to a ConvertRowtypeExpr that translates a wholerow of the child rel 
into a wholerow of the parent rel's rowtype.  This works well for the 
non-partitioned inheritance case, but makes complicated the code for 
handling the partitioning case especially when planning 
partitionwise-joins.  And I think this would be the root cause of this 
issue.  I don't think the tlists for the children need to have their 
wholerows transformed into the corresponding ConvertRowtypeExprs *at 
this point*, so what I'd like to propose is to 1) map a parent wholerow 
Var simply to a child wholerow Var, instead (otherwise, the same as 
adjust_appendrel_attrs), when building the reltarget for a child in 
set_append_rel_size, 2) build the tlists for child joins using those 
children's wholerow Vars at path creation time, and 3) adjust those 
wholerow Vars in the tlists for subpaths in the chosen AppendPath so 
that those are transformed into the corresponding ConvertRowtypeExprs, 
at plan creation time (ie, in 
create_append_plan/create_merge_append_plan).  IIUC, this would not 
require any changes to pull_var_clause as proposed by the patch.  This 
would not require any changes to postgres_fdw as proposed by the patch, 
either.  In addition, this would make unnecessary the code added to 
setrefs.c by the partitionwise-join patch.  Maybe I'm missing something 
though.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 7 Jun 2018 16:12:58 +0530

On Wed, Jun 6, 2018 at 5:00 PM, Etsuro Fujita
<[email-masked]> wrote:
> rethinking how to fix this issue.  Yet another idea I came up with to fix
> this is to redesign the handling of the tlists for children in the
> set_append_rel_size, which maps a wholerow Var referring to the parent rel
> to a ConvertRowtypeExpr that translates a wholerow of the child rel into a
> wholerow of the parent rel's rowtype.  This works well for the
> handling the partitioning case especially when planning partitionwise-joins.
> And I think this would be the root cause of this issue.

Although true, this is not only about targetlist. Even the whole-row
expressions in the conditions, equivalence classes and other
planner/optimizer data structures are translated to
ConvertRowtypeExpr.

>  I don't think the
> tlists for the children need to have their wholerows transformed into the
> corresponding ConvertRowtypeExprs *at this point*, so what I'd like to
> propose is to 1) map a parent wholerow Var simply to a child wholerow Var,
> instead (otherwise, the same as adjust_appendrel_attrs), when building the
> reltarget for a child in set_append_rel_size, 2) build the tlists for child
> joins using those children's wholerow Vars at path creation time, and 3)
> adjust those wholerow Vars in the tlists for subpaths in the chosen
> AppendPath so that those are transformed into the corresponding
> ConvertRowtypeExprs, at plan creation time (ie, in
> create_append_plan/create_merge_append_plan).  IIUC, this would not require
> any changes to pull_var_clause as proposed by the patch.  This would not
> require any changes to postgres_fdw as proposed by the patch, either.  In
> addition, this would make unnecessary the code added to setrefs.c by the
> partitionwise-join patch.  Maybe I'm missing something though.

Not translating whole-row expressions to ConvertRowtypeExpr before
creating paths can lead to a number of anomalies. For example,

1. if there's an index on the whole-row expression of child,
translating parent's whole-row expression to child's whole-row
expression as is will lead to using that index, when in reality the it
can not be used since the condition/ORDER BY clause (which originally
referred the parent's whole-row expression) require the child's
whole-row reassembled as parent's whole-row.
2. Constraints on child'd whole-row expression, will be used to prune
a child when they can not be used since the original condition was on
parent' whole-row expression and not that of the child.
3. Equivalence classes will think that a child whole-row expression
(without ConvertRowtypeExpr) is equivalent to an expression which is
part of the same equivalence class as the parent' whole-row
expression.

Given these serious problems, I don't think we could afford not to
cover a child whole-row reference in ConvertRowtypeExpr.

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 11 Jun 2018 19:35:29 +0900

> planner/optimizer data structures are translated to

Yeah, but I mean we modify set_append_rel_size so that we only map a 
parent wholerow Var in the parent tlist to a child wholerow Var in the 
child's tlist; parent wholerow Vars in the parent's other expressions 
such as conditions are transformed into CREs as-is.

> part of the same equivalence class as the parent' whole-row

Is that still possible when we only map a parent wholerow Var in the 
parent's tlist to a child wholerow Var in the child's tlist?

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 14 Jun 2018 19:07:07 +0530

On Mon, Jun 11, 2018 at 4:05 PM, Etsuro Fujita
<[email-masked]> wrote:
>>> rel
>>> partitionwise-joins.
> Yeah, but I mean we modify set_append_rel_size so that we only map a parent
> wholerow Var in the parent tlist to a child wholerow Var in the child's
> tlist; parent wholerow Vars in the parent's other expressions such as
> conditions are transformed into CREs as-is.

What happens to a PlaceHolderVar containing whole-row reference when
that appears in a condition and/or targetlist.

>>> require

Yes. 1 will affect the choice of index-only scans. We use pathkey
comparisons at a number of places so tlist going out of sync with
equivalence classes can affect a number of places.

build_tlist_to_deparse() is used to deparse targetlist at the time of
creating paths,as well as during the planning. According to your
proposal we will build different tlists during path creation and plan
creation. That doesn't look good. Apart from that your proposal of
changing a child's targetlist at the time of plan creation to use CRE
doesn't help since the original problem as described in [1] happens at
the time of creating plans as described in [1].

The parent's whole-row reference has a different data type than
child's whole-row reference. If we do not cover child's whole-row
reference by CRE, the parent and child targetlists will go out of sync
as far as the type of individual columns are concerned. That too
doesn't look good to me.

[1] https://www.postgresql.org/message-id/CAFjFpRfD5=[email-masked]

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 15 Jun 2018 20:56:20 +0900

> that appears in a condition and/or targetlist.

Whole-row Vars contained in such PHV's expressions will be mapped into 
ConvertRowtypeExprs with adjust_appendrel_attrs.

> equivalence classes can affect a number of places.

Sorry, I'm still not sure if #1 is really a problem.  Could you show me 
an example for that?

> creation. That doesn't look good.

Maybe my explanation was not enough, but my proposal will guarantee that 
the FDW can build the same tlist at path creation time and plan creation 
time because the RelOptInfo's reltarget from which the tlist is created 
doesn't change at all.

> Apart from that your proposal of

I think my proposal will address the original issue.  Actually, I've 
created a patch implementing that proposal.  That's still WIP, but it 
works well even for these cases (and makes code much more simple, as 
mentioned above!)  But I think that patch needs more work, so I'm 
planning to post it next week.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 19 Jun 2018 21:46:17 +0900

(2018/06/15 20:56), Etsuro Fujita wrote:
> Actually, I've
> created a patch implementing that proposal.

> But I think that patch needs more work, so I'm

Here is a patch for that.

* As I said upthread, the patch makes code much more simple; I removed 
all the changes to setrefs.c added by the partitionwise-join patch.  I 
also simplified the logic for building a tlist for a child-join rel. 
The original PWJ computes attr_needed data even for child rels, and 
build the tlist for a child-join by passing to build_joinrel_tlist that 
data for input child rels for the child-join.  But I think that's 
redundant, and it's more straightforward to apply adjust_appendrel_attrs 
to the parent-join's tlist to get the child-join's tlist.  So, I changed 
that way, which made unnecessary all the changes to build_joinrel_tlist 
and placeholder.c added by the PWJ patch, so I removed those as well.

* The patch contains all of the regression tests in the original patch 
proposed by Ashutosh.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 8215,8222 **** ALTER TABLE fprt2_p1 SET (autovacuum_enabled = 'false');
  ALTER TABLE fprt2_p2 SET (autovacuum_enabled = 'false');
  INSERT INTO fprt2_p1 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(0, 249, 3) i;
  INSERT INTO fprt2_p2 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(250, 499, 3) i;
! CREATE FOREIGN TABLE ftprt2_p1 PARTITION OF fprt2 FOR VALUES FROM (0) TO (250)
  	SERVER loopback OPTIONS (table_name 'fprt2_p1', use_remote_estimate 'true');
  CREATE FOREIGN TABLE ftprt2_p2 PARTITION OF fprt2 FOR VALUES FROM (250) TO (500)
  	SERVER loopback OPTIONS (table_name 'fprt2_p2', use_remote_estimate 'true');
  ANALYZE fprt2;
--- 8215,8223 ----
  ALTER TABLE fprt2_p2 SET (autovacuum_enabled = 'false');
  INSERT INTO fprt2_p1 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(0, 249, 3) i;
  INSERT INTO fprt2_p2 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(250, 499, 3) i;
! CREATE FOREIGN TABLE ftprt2_p1 (b int, c varchar, a int)
  	SERVER loopback OPTIONS (table_name 'fprt2_p1', use_remote_estimate 'true');
+ ALTER TABLE fprt2 ATTACH PARTITION ftprt2_p1 FOR VALUES FROM (0) TO (250);
  CREATE FOREIGN TABLE ftprt2_p2 PARTITION OF fprt2 FOR VALUES FROM (250) TO (500)
  	SERVER loopback OPTIONS (table_name 'fprt2_p2', use_remote_estimate 'true');
  ANALYZE fprt2;
***************
*** 8269,8294 **** SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * FROM fprt2 WHERE a < 10)
  
  -- with whole-row reference
  EXPLAIN (COSTS OFF)
! SELECT t1,t2 FROM fprt1 t1 JOIN fprt2 t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a % 25 =0 ORDER BY 1,2;
!                                    QUERY PLAN                                    
! ---------------------------------------------------------------------------------
   Sort
     Sort Key: ((t1.*)::fprt1), ((t2.*)::fprt2)
     ->  Append
           ->  Foreign Scan
!                Relations: (public.ftprt1_p1 t1) INNER JOIN (public.ftprt2_p1 t2)
           ->  Foreign Scan
!                Relations: (public.ftprt1_p2 t1) INNER JOIN (public.ftprt2_p2 t2)
  (7 rows)
  
! SELECT t1,t2 FROM fprt1 t1 JOIN fprt2 t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a % 25 =0 ORDER BY 1,2;
!        t1       |       t2       
  ----------------+----------------
   (0,0,0000)     | (0,0,0000)
   (150,150,0003) | (150,150,0003)
   (250,250,0005) | (250,250,0005)
   (400,400,0008) | (400,400,0008)
! (4 rows)
  
  -- join with lateral reference
  EXPLAIN (COSTS OFF)
--- 8270,8305 ----
  
  -- with whole-row reference
  EXPLAIN (COSTS OFF)
! SELECT t1.wr, t2.wr FROM (SELECT t1 wr, a FROM fprt1 t1 WHERE t1.a % 25 = 0) t1 FULL JOIN (SELECT t2 wr, b FROM fprt2 t2 WHERE t2.b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY 1,2;
!                                    QUERY PLAN                                   
! --------------------------------------------------------------------------------
   Sort
     Sort Key: ((t1.*)::fprt1), ((t2.*)::fprt2)
     ->  Append
           ->  Foreign Scan
!                Relations: (public.ftprt1_p1 t1) FULL JOIN (public.ftprt2_p1 t2)
           ->  Foreign Scan
!                Relations: (public.ftprt1_p2 t1) FULL JOIN (public.ftprt2_p2 t2)
  (7 rows)
  
! SELECT t1.wr, t2.wr FROM (SELECT t1 wr, a FROM fprt1 t1 WHERE t1.a % 25 = 0) t1 FULL JOIN (SELECT t2 wr, b FROM fprt2 t2 WHERE t2.b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY 1,2;
!        wr       |       wr       
  ----------------+----------------
   (0,0,0000)     | (0,0,0000)
+  (50,50,0001)   | 
+  (100,100,0002) | 
   (150,150,0003) | (150,150,0003)
+  (200,200,0004) | 
   (250,250,0005) | (250,250,0005)
+  (300,300,0006) | 
+  (350,350,0007) | 
   (400,400,0008) | (400,400,0008)
!  (450,450,0009) | 
!                 | (75,75,0001)
!                 | (225,225,0004)
!                 | (325,325,0006)
!                 | (475,475,0009)
! (14 rows)
  
  -- join with lateral reference
  EXPLAIN (COSTS OFF)
***************
*** 8352,8357 **** SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE
--- 8363,8398 ----
       |        | 475 | t2_phv
  (14 rows)
  
+ -- test FOR UPDATE
+ EXPLAIN (COSTS OFF)
+ SELECT t1.a, t2.b FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a = t2.b) WHERE t1.a % 25 = 0 ORDER BY 1,2 FOR UPDATE OF t1;
+                                       QUERY PLAN                                       
+ ---------------------------------------------------------------------------------------
+  LockRows
+    ->  Sort
+          Sort Key: t1.a
+          ->  Append
+                ->  Foreign Scan
+                      Relations: (public.ftprt1_p1 t1) INNER JOIN (public.ftprt2_p1 t2)
+                      ->  Nested Loop
+                            ->  Foreign Scan on ftprt1_p1 t1
+                            ->  Foreign Scan on ftprt2_p1 t2
+                ->  Foreign Scan
+                      Relations: (public.ftprt1_p2 t1) INNER JOIN (public.ftprt2_p2 t2)
+                      ->  Nested Loop
+                            ->  Foreign Scan on ftprt1_p2 t1_1
+                            ->  Foreign Scan on ftprt2_p2 t2_1
+ (14 rows)
+ 
+ SELECT t1.a, t2.b FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a = t2.b) WHERE t1.a % 25 = 0 ORDER BY 1,2 FOR UPDATE OF t1;
+   a  |  b  
+ -----+-----
+    0 |   0
+  150 | 150
+  250 | 250
+  400 | 400
+ (4 rows)
+ 
  RESET enable_partitionwise_join;
  -- ===================================================================
  -- test partitionwise aggregates
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 2216,2223 **** ALTER TABLE fprt2_p1 SET (autovacuum_enabled = 'false');
  ALTER TABLE fprt2_p2 SET (autovacuum_enabled = 'false');
  INSERT INTO fprt2_p1 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(0, 249, 3) i;
  INSERT INTO fprt2_p2 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(250, 499, 3) i;
! CREATE FOREIGN TABLE ftprt2_p1 PARTITION OF fprt2 FOR VALUES FROM (0) TO (250)
  	SERVER loopback OPTIONS (table_name 'fprt2_p1', use_remote_estimate 'true');
  CREATE FOREIGN TABLE ftprt2_p2 PARTITION OF fprt2 FOR VALUES FROM (250) TO (500)
  	SERVER loopback OPTIONS (table_name 'fprt2_p2', use_remote_estimate 'true');
  ANALYZE fprt2;
--- 2216,2224 ----
  ALTER TABLE fprt2_p2 SET (autovacuum_enabled = 'false');
  INSERT INTO fprt2_p1 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(0, 249, 3) i;
  INSERT INTO fprt2_p2 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(250, 499, 3) i;
! CREATE FOREIGN TABLE ftprt2_p1 (b int, c varchar, a int)
  	SERVER loopback OPTIONS (table_name 'fprt2_p1', use_remote_estimate 'true');
+ ALTER TABLE fprt2 ATTACH PARTITION ftprt2_p1 FOR VALUES FROM (0) TO (250);
  CREATE FOREIGN TABLE ftprt2_p2 PARTITION OF fprt2 FOR VALUES FROM (250) TO (500)
  	SERVER loopback OPTIONS (table_name 'fprt2_p2', use_remote_estimate 'true');
  ANALYZE fprt2;
***************
*** 2236,2243 **** SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * FROM fprt2 WHERE a < 10)
  
  -- with whole-row reference
  EXPLAIN (COSTS OFF)
! SELECT t1,t2 FROM fprt1 t1 JOIN fprt2 t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a % 25 =0 ORDER BY 1,2;
! SELECT t1,t2 FROM fprt1 t1 JOIN fprt2 t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a % 25 =0 ORDER BY 1,2;
  
  -- join with lateral reference
  EXPLAIN (COSTS OFF)
--- 2237,2244 ----
  
  -- with whole-row reference
  EXPLAIN (COSTS OFF)
! SELECT t1.wr, t2.wr FROM (SELECT t1 wr, a FROM fprt1 t1 WHERE t1.a % 25 = 0) t1 FULL JOIN (SELECT t2 wr, b FROM fprt2 t2 WHERE t2.b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY 1,2;
! SELECT t1.wr, t2.wr FROM (SELECT t1 wr, a FROM fprt1 t1 WHERE t1.a % 25 = 0) t1 FULL JOIN (SELECT t2 wr, b FROM fprt2 t2 WHERE t2.b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY 1,2;
  
  -- join with lateral reference
  EXPLAIN (COSTS OFF)
***************
*** 2249,2254 **** EXPLAIN (COSTS OFF)
--- 2250,2260 ----
  SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
  SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
  
+ -- test FOR UPDATE
+ EXPLAIN (COSTS OFF)
+ SELECT t1.a, t2.b FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a = t2.b) WHERE t1.a % 25 = 0 ORDER BY 1,2 FOR UPDATE OF t1;
+ SELECT t1.a, t2.b FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a = t2.b) WHERE t1.a % 25 = 0 ORDER BY 1,2 FOR UPDATE OF t1;
+ 
  RESET enable_partitionwise_join;

*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***************
*** 956,1027 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  		childrel = find_base_rel(root, childRTindex);
  		Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL);
  
! 		if (rel->part_scheme)
  		{
- 			AttrNumber	attno;
- 
  			/*
! 			 * We need attr_needed data for building targetlist of a join
! 			 * relation representing join between matching partitions for
! 			 * partitionwise join. A given attribute of a child will be needed
! 			 * in the same highest joinrel where the corresponding attribute
! 			 * of parent is needed. Hence it suffices to use the same Relids
! 			 * set for parent and child.
  			 */
! 			for (attno = rel->min_attr; attno <= rel->max_attr; attno++)
! 			{
! 				int			index = attno - rel->min_attr;
! 				Relids		attr_needed = rel->attr_needed[index];
! 
! 				/* System attributes do not need translation. */
! 				if (attno <= 0)
! 				{
! 					Assert(rel->min_attr == childrel->min_attr);
! 					childrel->attr_needed[index] = attr_needed;
! 				}
! 				else
! 				{
! 					Var		   *var = list_nth_node(Var,
! 													appinfo->translated_vars,
! 													attno - 1);
! 					int			child_index;
! 
! 					/*
! 					 * Ignore any column dropped from the parent.
! 					 * Corresponding Var won't have any translation. It won't
! 					 * have attr_needed information, since it can not be
! 					 * referenced in the query.
! 					 */
! 					if (var == NULL)
! 					{
! 						Assert(attr_needed == NULL);
! 						continue;
! 					}
! 
! 					child_index = var->varattno - childrel->min_attr;
! 					childrel->attr_needed[child_index] = attr_needed;
! 				}
! 			}
  		}
  
  		/*
- 		 * Copy/Modify targetlist. Even if this child is deemed empty, we need
- 		 * its targetlist in case it falls on nullable side in a child-join
- 		 * because of partitionwise join.
- 		 *
- 		 * NB: the resulting childrel->reltarget->exprs may contain arbitrary
- 		 * expressions, which otherwise would not occur in a rel's targetlist.
- 		 * Code that might be looking at an appendrel child must cope with
- 		 * such.  (Normally, a rel's targetlist would only include Vars and
- 		 * PlaceHolderVars.)  XXX we do not bother to update the cost or width
- 		 * fields of childrel->reltarget; not clear if that would be useful.
- 		 */
- 		childrel->reltarget->exprs = (List *)
- 			adjust_appendrel_attrs(root,
- 								   (Node *) rel->reltarget->exprs,
- 								   1, &appinfo);
- 
- 		/*
  		 * We have to make child entries in the EquivalenceClass data
  		 * structures as well.  This is needed either if the parent
  		 * participates in some eclass joins (because we will want to consider
--- 956,996 ----
  		childrel = find_base_rel(root, childRTindex);
  		Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL);
  
! 		/*
! 		 * We have to copy the parent's targetlist to the child, with
! 		 * appropriate substitution of variables.
! 		 *
! 		 * We need special handling for partitioned inheritance cases if
! 		 * partitionwise join technique is enabled: if the entry in the parent
! 		 * rel's targetlist is a whole-row Var for the parent rel, map it into
! 		 * a whole-row Var for the child rel (we convert the tuple layout back
! 		 * to the parent's rowtype at plan creation time); otherwise, map
! 		 * expressions in the targetlist with adjust_appendrel_attrs.  Note:
! 		 * even if the child is deemed empty, we need the targetlist for it in
! 		 * case it falls on nullable side in a child-join.
! 		 */
! 		if (enable_partitionwise_join && rel->top_parent_is_partitioned)
! 		{
! 			build_childrel_tlist(root, rel, childrel, 1, &appinfo);
! 		}
! 		else
  		{
  			/*
! 			 * NB: the resulting childrel->reltarget->exprs may contain
! 			 * arbitrary expressions, which otherwise would not occur in a
! 			 * rel's targetlist.  Code that might be looking at an appendrel
! 			 * child must cope with such.  (Normally, a rel's targetlist would
! 			 * only include Vars and PlaceHolderVars.)  XXX we do not bother
! 			 * to update the cost or width fields of childrel->reltarget; not
! 			 * clear if that would be useful.
  			 */
! 			childrel->reltarget->exprs = (List *)
! 				adjust_appendrel_attrs(root,
! 									   (Node *) rel->reltarget->exprs,
! 									   1, &appinfo);
  		}
  
  		/*
  		 * We have to make child entries in the EquivalenceClass data
  		 * structures as well.  This is needed either if the parent
  		 * participates in some eclass joins (because we will want to consider
***************
*** 1181,1186 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
--- 1150,1163 ----
  								   1, &appinfo);
  
  		/*
+ 		 * Note: we could compute appropriate attr_needed data for the child's
+ 		 * variables, by transforming the parent's attr_needed through the
+ 		 * translated_vars mapping.  However, currently there's no need
+ 		 * because attr_needed is only examined for base relations not
+ 		 * otherrels.  So we just leave the child's attr_needed empty.
+ 		 */
+ 
+ 		/*
  		 * If parallelism is allowable for this query in general, see whether
  		 * it's allowable for this childrel in particular.  But if we've
  		 * already decided the appendrel is not parallel-safe as a whole,
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***************
*** 29,34 ****
--- 29,35 ----
  #include "nodes/nodeFuncs.h"
  #include "optimizer/clauses.h"
  #include "optimizer/cost.h"
+ #include "optimizer/pathnode.h"
  #include "optimizer/paths.h"
  #include "optimizer/placeholder.h"
  #include "optimizer/plancat.h"
***************
*** 171,176 **** static void copy_generic_path_info(Plan *dest, Path *src);
--- 172,178 ----
  static void copy_plan_costsize(Plan *dest, Plan *src);
  static void label_sort_with_costsize(PlannerInfo *root, Sort *plan,
  						 double limit_tuples);
+ static void adjust_subplan_tlist(PlannerInfo *root, RelOptInfo *rel, Plan *subplan);
  static SeqScan *make_seqscan(List *qptlist, List *qpqual, Index scanrelid);
  static SampleScan *make_samplescan(List *qptlist, List *qpqual, Index scanrelid,
  				TableSampleClause *tsc);
***************
*** 1074,1079 **** create_append_plan(PlannerInfo *root, AppendPath *best_path)
--- 1076,1084 ----
  		/* Must insist that all children return the same tlist */
  		subplan = create_plan_recurse(root, subpath, CP_EXACT_TLIST);
  
+ 		/* Adjust the plan's targetlist (if necessary) */
+ 		adjust_subplan_tlist(root, subpath->parent, subplan);
+ 
  		subplans = lappend(subplans, subplan);
  	}
  
***************
*** 1185,1190 **** create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path)
--- 1190,1198 ----
  		/* Must insist that all children return the same tlist */
  		subplan = create_plan_recurse(root, subpath, CP_EXACT_TLIST);
  
+ 		/* Adjust the plan's targetlist (if necessary) */
+ 		adjust_subplan_tlist(root, subpath->parent, subplan);
+ 
  		/* Compute sort column info, and adjust subplan's tlist as needed */
  		subplan = prepare_sort_from_pathkeys(subplan, pathkeys,
  											 subpath->parent->relids,
***************
*** 5074,5079 **** bitmap_subplan_mark_shared(Plan *plan)
--- 5082,5144 ----
  		elog(ERROR, "unrecognized node type: %d", nodeTag(plan));
  }
  
+ /*
+  * adjust_subplan_tlist
+  *	  If the child plan's targetlist has a whole-row Var as an entry of the
+  *	  list, add a ConvertRowtypeExpr above the Var to convert the tuple layout
+  *	  back to the top parent's rowtype.
+  */
+ static void
+ adjust_subplan_tlist(PlannerInfo *root, RelOptInfo *rel, Plan *subplan)
+ {
+ 	List	   *newtlist;
+ 	ListCell   *l;
+ 
+ 	/* No work if the given rel's tlist doesn't have whole-row Vars */
+ 	if (!rel->has_child_wholerow)
+ 		return;
+ 
+ 	/* The given rel should be a scan or join relation */
+ 	Assert(rel->reloptkind == RELOPT_OTHER_MEMBER_REL ||
+ 		   rel->reloptkind == RELOPT_OTHER_JOINREL);
+ 
+ 	/* No work if the child plan is an Append or MergeAppend */
+ 	if (IsA(subplan, Append) || IsA(subplan, MergeAppend))
+ 		return;
+ 
+ 	/* The child plan should be able to do projection */
+ 	Assert(is_projection_capable_plan(subplan));
+ 
+ 	newtlist = NIL;
+ 	foreach(l, subplan->targetlist)
+ 	{
+ 		TargetEntry *tle = (TargetEntry *) lfirst(l);
+ 		Var		   *var = (Var *) tle->expr;
+ 
+ 		if (IsA(var, Var) && var->varattno == 0)
+ 		{
+ 			RelOptInfo *partrel = find_base_rel(root, var->varno);
+ 			ConvertRowtypeExpr *cre = makeNode(ConvertRowtypeExpr);
+ 
+ 			/* The rel should be a partition */
+ 			Assert(partrel->reloptkind == RELOPT_OTHER_MEMBER_REL);
+ 
+ 			/* The top parent rel should be a partitioned table */
+ 			Assert(partrel->top_parent_is_partitioned);
+ 
+ 			cre->arg = (Expr *) var;
+ 			cre->resulttype = partrel->top_parent_rowtype;
+ 			cre->convertformat = COERCE_IMPLICIT_CAST;
+ 			cre->location = -1;
+ 
+ 			tle = flatCopyTargetEntry(tle);
+ 			tle->expr = (Expr *) cre;
+ 		}
+ 		newtlist = lappend(newtlist, tle);
+ 	}
+ 	subplan->targetlist = newtlist;
+ }
+ 
  /*****************************************************************************
   *
   *	PLAN NODE BUILDING ROUTINES
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
***************
*** 41,49 **** typedef struct
  	int			num_vars;		/* number of plain Var tlist entries */
  	bool		has_ph_vars;	/* are there PlaceHolderVar entries? */
  	bool		has_non_vars;	/* are there other entries? */
- 	bool		has_conv_whole_rows;	/* are there ConvertRowtypeExpr
- 										 * entries encapsulating a whole-row
- 										 * Var? */
  	tlist_vinfo vars[FLEXIBLE_ARRAY_MEMBER];	/* has num_vars entries */
  } indexed_tlist;
  
--- 41,46 ----
***************
*** 143,149 **** static List *set_returning_clause_references(PlannerInfo *root,
  								int rtoffset);
  static bool extract_query_dependencies_walker(Node *node,
  								  PlannerInfo *context);
- static bool is_converted_whole_row_reference(Node *node);
  
  /*****************************************************************************
   *
--- 140,145 ----
***************
*** 1997,2003 **** build_tlist_index(List *tlist)
  	itlist->tlist = tlist;
  	itlist->has_ph_vars = false;
  	itlist->has_non_vars = false;
- 	itlist->has_conv_whole_rows = false;
  
  	/* Find the Vars and fill in the index array */
  	vinfo = itlist->vars;
--- 1993,1998 ----
***************
*** 2016,2023 **** build_tlist_index(List *tlist)
  		}
  		else if (tle->expr && IsA(tle->expr, PlaceHolderVar))
  			itlist->has_ph_vars = true;
- 		else if (is_converted_whole_row_reference((Node *) tle->expr))
- 			itlist->has_conv_whole_rows = true;
  		else
  			itlist->has_non_vars = true;
  	}
--- 2011,2016 ----
***************
*** 2033,2042 **** build_tlist_index(List *tlist)
   * This is like build_tlist_index, but we only index tlist entries that
   * are Vars belonging to some rel other than the one specified.  We will set
   * has_ph_vars (allowing PlaceHolderVars to be matched), but not has_non_vars
!  * (so nothing other than Vars and PlaceHolderVars can be matched). In case of
!  * DML, where this function will be used, returning lists from child relations
!  * will be appended similar to a simple append relation. That does not require
!  * fixing ConvertRowtypeExpr references. So, those are not considered here.
   */
  static indexed_tlist *
  build_tlist_index_other_vars(List *tlist, Index ignore_rel)
--- 2026,2032 ----
   * This is like build_tlist_index, but we only index tlist entries that
   * are Vars belonging to some rel other than the one specified.  We will set
   * has_ph_vars (allowing PlaceHolderVars to be matched), but not has_non_vars
!  * (so nothing other than Vars and PlaceHolderVars can be matched).
   */
  static indexed_tlist *
  build_tlist_index_other_vars(List *tlist, Index ignore_rel)
***************
*** 2053,2059 **** build_tlist_index_other_vars(List *tlist, Index ignore_rel)
  	itlist->tlist = tlist;
  	itlist->has_ph_vars = false;
  	itlist->has_non_vars = false;
- 	itlist->has_conv_whole_rows = false;
  
  	/* Find the desired Vars and fill in the index array */
  	vinfo = itlist->vars;
--- 2043,2048 ----
***************
*** 2256,2262 **** static Node *
  fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
  {
  	Var		   *newvar;
- 	bool		converted_whole_row;
  
  	if (node == NULL)
  		return NULL;
--- 2245,2250 ----
***************
*** 2326,2337 **** fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
  	}
  	if (IsA(node, Param))
  		return fix_param_node(context->root, (Param *) node);
- 
  	/* Try matching more complex expressions too, if tlists have any */
! 	converted_whole_row = is_converted_whole_row_reference(node);
! 	if (context->outer_itlist &&
! 		(context->outer_itlist->has_non_vars ||
! 		 (context->outer_itlist->has_conv_whole_rows && converted_whole_row)))
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->outer_itlist,
--- 2314,2321 ----
  	}
  	if (IsA(node, Param))
  		return fix_param_node(context->root, (Param *) node);
  	/* Try matching more complex expressions too, if tlists have any */
! 	if (context->outer_itlist && context->outer_itlist->has_non_vars)
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->outer_itlist,
***************
*** 2339,2347 **** fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
  		if (newvar)
  			return (Node *) newvar;
  	}
! 	if (context->inner_itlist &&
! 		(context->inner_itlist->has_non_vars ||
! 		 (context->inner_itlist->has_conv_whole_rows && converted_whole_row)))
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->inner_itlist,
--- 2323,2329 ----
  		if (newvar)
  			return (Node *) newvar;
  	}
! 	if (context->inner_itlist && context->inner_itlist->has_non_vars)
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->inner_itlist,
***************
*** 2461,2469 **** fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
  		/* If no match, just fall through to process it normally */
  	}
  	/* Try matching more complex expressions too, if tlist has any */
! 	if (context->subplan_itlist->has_non_vars ||
! 		(context->subplan_itlist->has_conv_whole_rows &&
! 		 is_converted_whole_row_reference(node)))
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->subplan_itlist,
--- 2443,2449 ----
  		/* If no match, just fall through to process it normally */
  	}
  	/* Try matching more complex expressions too, if tlist has any */
! 	if (context->subplan_itlist->has_non_vars)
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->subplan_itlist,
***************
*** 2670,2702 **** extract_query_dependencies_walker(Node *node, PlannerInfo *context)
  	return expression_tree_walker(node, extract_query_dependencies_walker,
  								  (void *) context);
  }
- 
- /*
-  * is_converted_whole_row_reference
-  *		If the given node is a ConvertRowtypeExpr encapsulating a whole-row
-  *		reference as implicit cast, return true. Otherwise return false.
-  */
- static bool
- is_converted_whole_row_reference(Node *node)
- {
- 	ConvertRowtypeExpr *convexpr;
- 
- 	if (!node || !IsA(node, ConvertRowtypeExpr))
- 		return false;
- 
- 	/* Traverse nested ConvertRowtypeExpr's. */
- 	convexpr = castNode(ConvertRowtypeExpr, node);
- 	while (convexpr->convertformat == COERCE_IMPLICIT_CAST &&
- 		   IsA(convexpr->arg, ConvertRowtypeExpr))
- 		convexpr = castNode(ConvertRowtypeExpr, convexpr->arg);
- 
- 	if (IsA(convexpr->arg, Var))
- 	{
- 		Var		   *var = castNode(Var, convexpr->arg);
- 
- 		if (var->varattno == 0)
- 			return true;
- 	}
- 
- 	return false;
- }
--- 2650,2652 ----
*** a/src/backend/optimizer/util/placeholder.c
--- b/src/backend/optimizer/util/placeholder.c
***************
*** 20,26 ****
  #include "optimizer/pathnode.h"
  #include "optimizer/placeholder.h"
  #include "optimizer/planmain.h"
- #include "optimizer/prep.h"
  #include "optimizer/var.h"
  #include "utils/lsyscache.h"
  
--- 20,25 ----
***************
*** 415,424 **** add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
  	Relids		relids = joinrel->relids;
  	ListCell   *lc;
  
- 	/* This function is called only on the parent relations. */
- 	Assert(!IS_OTHER_REL(joinrel) && !IS_OTHER_REL(outer_rel) &&
- 		   !IS_OTHER_REL(inner_rel));
- 
  	foreach(lc, root->placeholder_list)
  	{
  		PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(lc);
--- 414,419 ----
***************
*** 464,519 **** add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
  		}
  	}
  }
- 
- /*
-  * add_placeholders_to_child_joinrel
-  *		Translate the PHVs in parent's targetlist and add them to the child's
-  *		targetlist. Also adjust the cost
-  */
- void
- add_placeholders_to_child_joinrel(PlannerInfo *root, RelOptInfo *childrel,
- 								  RelOptInfo *parentrel)
- {
- 	ListCell   *lc;
- 	AppendRelInfo **appinfos;
- 	int			nappinfos;
- 
- 	Assert(IS_JOIN_REL(childrel) && IS_JOIN_REL(parentrel));
- 	Assert(IS_OTHER_REL(childrel));
- 
- 	/* Nothing to do if no PHVs. */
- 	if (root->placeholder_list == NIL)
- 		return;
- 
- 	appinfos = find_appinfos_by_relids(root, childrel->relids, &nappinfos);
- 	foreach(lc, parentrel->reltarget->exprs)
- 	{
- 		PlaceHolderVar *phv = lfirst(lc);
- 
- 		if (IsA(phv, PlaceHolderVar))
- 		{
- 			/*
- 			 * In case the placeholder Var refers to any of the parent
- 			 * relations, translate it to refer to the corresponding child.
- 			 */
- 			if (bms_overlap(phv->phrels, parentrel->relids) &&
- 				childrel->reloptkind == RELOPT_OTHER_JOINREL)
- 			{
- 				phv = (PlaceHolderVar *) adjust_appendrel_attrs(root,
- 																(Node *) phv,
- 																nappinfos,
- 																appinfos);
- 			}
- 
- 			childrel->reltarget->exprs = lappend(childrel->reltarget->exprs,
- 												 phv);
- 		}
- 	}
- 
- 	/* Adjust the cost and width of child targetlist. */
- 	childrel->reltarget->cost.startup = parentrel->reltarget->cost.startup;
- 	childrel->reltarget->cost.per_tuple = parentrel->reltarget->cost.per_tuple;
- 	childrel->reltarget->width = parentrel->reltarget->width;
- 
- 	pfree(appinfos);
- }
--- 459,461 ----
*** a/src/backend/optimizer/util/relnode.c
--- b/src/backend/optimizer/util/relnode.c
***************
*** 16,21 ****
--- 16,22 ----
  
  #include <limits.h>
  
+ #include "catalog/pg_class.h"
  #include "miscadmin.h"
  #include "optimizer/clauses.h"
  #include "optimizer/cost.h"
***************
*** 28,33 ****
--- 29,35 ----
  #include "optimizer/tlist.h"
  #include "partitioning/partbounds.h"
  #include "utils/hsearch.h"
+ #include "utils/lsyscache.h"

  typedef struct JoinHashEntry
***************
*** 57,62 **** static void add_join_rel(PlannerInfo *root, RelOptInfo *joinrel);
--- 59,69 ----
  static void build_joinrel_partition_info(RelOptInfo *joinrel,
  							 RelOptInfo *outer_rel, RelOptInfo *inner_rel,
  							 List *restrictlist, JoinType jointype);
+ static void build_child_join_reltarget(PlannerInfo *root,
+ 						   RelOptInfo *parentrel,
+ 						   RelOptInfo *childrel,
+ 						   int nappinfos,
+ 						   AppendRelInfo **appinfos);

  /*
***************
*** 151,156 **** build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
--- 158,166 ----
  	rel->baserestrict_min_security = UINT_MAX;
  	rel->joininfo = NIL;
  	rel->has_eclass_joins = false;
+ 	rel->top_parent_is_partitioned = false;
+ 	rel->top_parent_rowtype = InvalidOid;
+ 	rel->has_child_wholerow = false;
  	rel->part_scheme = NULL;
  	rel->nparts = 0;
  	rel->boundinfo = NULL;
***************
*** 165,170 **** build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
--- 175,183 ----
  	 * has top_parent_relids set, it's a direct or an indirect child of the
  	 * top parent indicated by top_parent_relids. By extension this child is
  	 * also an indirect child of that parent.
+ 	 *
+ 	 * Also pass the flag to indicate whether the top parent is a partitioned
+ 	 * table and the OID of the top parent rowtype down the hierarchy.
  	 */
  	if (parent)
  	{
***************
*** 172,181 **** build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
--- 185,205 ----
  			rel->top_parent_relids = parent->top_parent_relids;
  		else
  			rel->top_parent_relids = bms_copy(parent->relids);
+ 
+ 		rel->top_parent_is_partitioned = parent->top_parent_is_partitioned;
+ 		rel->top_parent_rowtype = parent->top_parent_rowtype;
  	}
  	else
+ 	{
  		rel->top_parent_relids = NULL;
  
+ 		if (rte->relkind == RELKIND_PARTITIONED_TABLE)
+ 		{
+ 			rel->top_parent_is_partitioned = true;
+ 			rel->top_parent_rowtype = get_rel_type_id(rte->relid);
+ 		}
+ 	}
+ 
  	/* Check type of rtable entry */
  	switch (rte->rtekind)
  	{
***************
*** 565,571 **** build_join_rel(PlannerInfo *root,
--- 589,598 ----
  	joinrel->baserestrict_min_security = UINT_MAX;
  	joinrel->joininfo = NIL;
  	joinrel->has_eclass_joins = false;
+ 	joinrel->top_parent_is_partitioned = false;
+ 	joinrel->top_parent_rowtype = InvalidOid;
  	joinrel->top_parent_relids = NULL;
+ 	joinrel->has_child_wholerow = false;
  	joinrel->part_scheme = NULL;
  	joinrel->nparts = 0;
  	joinrel->boundinfo = NULL;
***************
*** 736,742 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
--- 763,772 ----
  	joinrel->baserestrictcost.per_tuple = 0;
  	joinrel->joininfo = NIL;
  	joinrel->has_eclass_joins = false;
+ 	joinrel->top_parent_is_partitioned = false;
+ 	joinrel->top_parent_rowtype = InvalidOid;
  	joinrel->top_parent_relids = NULL;
+ 	joinrel->has_child_wholerow = false;
  	joinrel->part_scheme = NULL;
  	joinrel->nparts = 0;
  	joinrel->boundinfo = NULL;
***************
*** 752,765 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
  	/* Compute information relevant to foreign relations. */
  	set_foreign_rel_properties(joinrel, outer_rel, inner_rel);
  
! 	/* Build targetlist */
! 	build_joinrel_tlist(root, joinrel, outer_rel);
! 	build_joinrel_tlist(root, joinrel, inner_rel);
! 	/* Add placeholder variables. */
! 	add_placeholders_to_child_joinrel(root, joinrel, parent_joinrel);
  
  	/* Construct joininfo list. */
- 	appinfos = find_appinfos_by_relids(root, joinrel->relids, &nappinfos);
  	joinrel->joininfo = (List *) adjust_appendrel_attrs(root,
  														(Node *) parent_joinrel->joininfo,
  														nappinfos,
--- 782,794 ----
  	/* Compute information relevant to foreign relations. */
  	set_foreign_rel_properties(joinrel, outer_rel, inner_rel);
  
! 	appinfos = find_appinfos_by_relids(root, joinrel->relids, &nappinfos);
! 
! 	/* Set up reltarget struct */
! 	build_child_join_reltarget(root, parent_joinrel, joinrel,
! 							   nappinfos, appinfos);
  
  	/* Construct joininfo list. */
  	joinrel->joininfo = (List *) adjust_appendrel_attrs(root,
  														(Node *) parent_joinrel->joininfo,
  														nappinfos,
***************
*** 858,872 **** static void
  build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
  					RelOptInfo *input_rel)
  {
! 	Relids		relids;
  	ListCell   *vars;
  
- 	/* attrs_needed refers to parent relids and not those of a child. */
- 	if (joinrel->top_parent_relids)
- 		relids = joinrel->top_parent_relids;
- 	else
- 		relids = joinrel->relids;
- 
  	foreach(vars, input_rel->reltarget->exprs)
  	{
  		Var		   *var = (Var *) lfirst(vars);
--- 887,895 ----
  build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
  					RelOptInfo *input_rel)
  {
! 	Relids		relids = joinrel->relids;
  	ListCell   *vars;
  
  	foreach(vars, input_rel->reltarget->exprs)
  	{
  		Var		   *var = (Var *) lfirst(vars);
***************
*** 882,935 **** build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
  
  		/*
  		 * Otherwise, anything in a baserel or joinrel targetlist ought to be
! 		 * a Var. Children of a partitioned table may have ConvertRowtypeExpr
! 		 * translating whole-row Var of a child to that of the parent.
! 		 * Children of an inherited table or subquery child rels can not
! 		 * directly participate in a join, so other kinds of nodes here.
  		 */
! 		if (IsA(var, Var))
! 		{
! 			baserel = find_base_rel(root, var->varno);
! 			ndx = var->varattno - baserel->min_attr;
! 		}
! 		else if (IsA(var, ConvertRowtypeExpr))
! 		{
! 			ConvertRowtypeExpr *child_expr = (ConvertRowtypeExpr *) var;
! 			Var		   *childvar = (Var *) child_expr->arg;
! 
! 			/*
! 			 * Child's whole-row references are converted to look like those
! 			 * of parent using ConvertRowtypeExpr. There can be as many
! 			 * ConvertRowtypeExpr decorations as the depth of partition tree.
! 			 * The argument to the deepest ConvertRowtypeExpr is expected to
! 			 * be a whole-row reference of the child.
! 			 */
! 			while (IsA(childvar, ConvertRowtypeExpr))
! 			{
! 				child_expr = (ConvertRowtypeExpr *) childvar;
! 				childvar = (Var *) child_expr->arg;
! 			}
! 			Assert(IsA(childvar, Var) &&childvar->varattno == 0);
! 
! 			baserel = find_base_rel(root, childvar->varno);
! 			ndx = 0 - baserel->min_attr;
! 		}
! 		else
  			elog(ERROR, "unexpected node type in rel targetlist: %d",
  				 (int) nodeTag(var));

! 		/* Is the target expression still needed above this joinrel? */
  		if (bms_nonempty_difference(baserel->attr_needed[ndx], relids))
  		{
  			/* Yup, add it to the output */
  			joinrel->reltarget->exprs = lappend(joinrel->reltarget->exprs, var);
! 
! 			/*
! 			 * Vars have cost zero, so no need to adjust reltarget->cost. Even
! 			 * if it's a ConvertRowtypeExpr, it will be computed only for the
! 			 * base relation, costing nothing for a join.
! 			 */
  			joinrel->reltarget->width += baserel->attr_widths[ndx];
  		}
  	}
--- 905,927 ----
  
  		/*
  		 * Otherwise, anything in a baserel or joinrel targetlist ought to be
! 		 * a Var.  (More general cases can only appear in appendrel child
! 		 * rels, which will never be seen here.)
  		 */
! 		if (!IsA(var, Var))
  			elog(ERROR, "unexpected node type in rel targetlist: %d",
  				 (int) nodeTag(var));
  
+ 		/* Get the Var's original base rel */
+ 		baserel = find_base_rel(root, var->varno);
  
! 		/* Is it still needed above this joinrel? */
! 		ndx = var->varattno - baserel->min_attr;
  		if (bms_nonempty_difference(baserel->attr_needed[ndx], relids))
  		{
  			/* Yup, add it to the output */
  			joinrel->reltarget->exprs = lappend(joinrel->reltarget->exprs, var);
! 			/* Vars have cost zero, so no need to adjust reltarget->cost */
  			joinrel->reltarget->width += baserel->attr_widths[ndx];
  		}
  	}
***************
*** 1178,1183 **** fetch_upper_rel(PlannerInfo *root, UpperRelationKind kind, Relids relids)
--- 1170,1177 ----
  	upperrel->cheapest_unique_path = NULL;
  	upperrel->cheapest_parameterized_paths = NIL;
  
+ 	upperrel->has_child_wholerow = false;
+ 
  	root->upper_rels[kind] = lappend(root->upper_rels[kind], upperrel);
  
  	return upperrel;
***************
*** 1760,1762 **** build_joinrel_partition_info(RelOptInfo *joinrel, RelOptInfo *outer_rel,
--- 1754,1834 ----
  		joinrel->nullable_partexprs[cnt] = nullable_partexpr;
  	}
  }
+ 
+ /*
+  * build_childrel_tlist
+  *	  Build a child relation's targetlist from a parent relation.
+  */
+ void
+ build_childrel_tlist(PlannerInfo *root,
+ 					 RelOptInfo *parentrel,
+ 					 RelOptInfo *childrel,
+ 					 int nappinfos,
+ 					 AppendRelInfo **appinfos)
+ {
+ 	ListCell   *lc;
+ 
+ 	foreach(lc, parentrel->reltarget->exprs)
+ 	{
+ 		Node	   *node = (Node *) lfirst(lc);
+ 
+ 		/* Should be a simple Var or PlaceHolderVar ... */
+ 		Assert(IsA(node, Var) || IsA(node, PlaceHolderVar));
+ 
+ 		if (IsA(node, Var) && ((Var *) node)->varattno == 0)
+ 		{
+ 			Var		   *var = (Var *) copyObject(node);
+ 			AppendRelInfo *appinfo = NULL;
+ 			int			cnt;
+ 
+ 			/* Find the AppendRelInfo associated with the parent */
+ 			for (cnt = 0; cnt < nappinfos; cnt++)
+ 			{
+ 				if (var->varno == appinfos[cnt]->parent_relid)
+ 				{
+ 					appinfo = appinfos[cnt];
+ 					break;
+ 				}
+ 			}
+ 
+ 			/* Should have found the entry ... */
+ 			Assert(appinfo);
+ 
+ 			var->varno = appinfo->child_relid;
+ 			var->varnoold = appinfo->child_relid;
+ 			var->vartype = appinfo->child_reltype;
+ 
+ 			childrel->reltarget->exprs =
+ 				lappend(childrel->reltarget->exprs, var);
+ 			childrel->has_child_wholerow = true;
+ 		}
+ 		else
+ 		{
+ 			childrel->reltarget->exprs =
+ 				lappend(childrel->reltarget->exprs,
+ 						adjust_appendrel_attrs(root, node,
+ 											   nappinfos,
+ 											   appinfos));
+ 		}
+ 	}
+ }
+ 
+ /*
+  * build_child_join_reltarget
+  *	  Set up a child-join relation's reltarget from a parent-join relation.
+  */
+ static void
+ build_child_join_reltarget(PlannerInfo *root,
+ 						   RelOptInfo *parentrel,
+ 						   RelOptInfo *childrel,
+ 						   int nappinfos,
+ 						   AppendRelInfo **appinfos)
+ {
+ 	/* Build the targetlist */
+ 	build_childrel_tlist(root, parentrel, childrel, nappinfos, appinfos);
+ 
+ 	/* Set the cost and width fields */
+ 	childrel->reltarget->cost.startup = parentrel->reltarget->cost.startup;
+ 	childrel->reltarget->cost.per_tuple = parentrel->reltarget->cost.per_tuple;
+ 	childrel->reltarget->width = parentrel->reltarget->width;
+ }
*** a/src/include/nodes/relation.h
--- b/src/include/nodes/relation.h
***************
*** 678,684 **** typedef struct RelOptInfo
--- 678,689 ----
  	bool		has_eclass_joins;	/* T means joininfo is incomplete */
  
  	/* used by "other" relations */
+ 	bool		top_parent_is_partitioned;	/* is topmost parent a partitioned
+ 											 * table? */
+ 	Oid			top_parent_rowtype; /* OID of topmost parent's rowtype */
  	Relids		top_parent_relids;	/* Relids of topmost parents */
+ 	bool		has_child_wholerow;	/* does reltarget->exprs have child
+ 									 * whole-row Vars? */
  
  	/* used for partitioned relations */
  	PartitionScheme part_scheme;	/* Partitioning scheme. */
*** a/src/include/optimizer/pathnode.h
--- b/src/include/optimizer/pathnode.h
***************
*** 299,303 **** extern RelOptInfo *build_child_join_rel(PlannerInfo *root,
--- 299,308 ----
  					 RelOptInfo *outer_rel, RelOptInfo *inner_rel,
  					 RelOptInfo *parent_joinrel, List *restrictlist,
  					 SpecialJoinInfo *sjinfo, JoinType jointype);
+ extern void build_childrel_tlist(PlannerInfo *root,
+ 					 RelOptInfo *parentrel,
+ 					 RelOptInfo *childrel,
+ 					 int nappinfos,
+ 					 AppendRelInfo **appinfos);
  
  #endif							/* PATHNODE_H */
*** a/src/test/regress/expected/partition_join.out
--- b/src/test/regress/expected/partition_join.out
***************
*** 1042,1047 **** SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT *
--- 1042,1090 ----
   400 |    
  (9 rows)
  
+ -- merge join when expression with whole-row reference needs to be sorted
+ EXPLAIN (COSTS OFF)
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+                                     QUERY PLAN                                     
+ -----------------------------------------------------------------------------------
+  Merge Append
+    Sort Key: t1.a
+    ->  Merge Join
+          Merge Cond: ((t1.a = t2.b) AND (((t1.*)::text) = ((t2.*)::text)))
+          ->  Sort
+                Sort Key: t1.a, ((t1.*)::text)
+                ->  Seq Scan on prt1_p1 t1
+          ->  Sort
+                Sort Key: t2.b, ((t2.*)::text)
+                ->  Seq Scan on prt2_p1 t2
+    ->  Merge Join
+          Merge Cond: ((t1_1.a = t2_1.b) AND (((t1_1.*)::text) = ((t2_1.*)::text)))
+          ->  Sort
+                Sort Key: t1_1.a, ((t1_1.*)::text)
+                ->  Seq Scan on prt1_p2 t1_1
+          ->  Sort
+                Sort Key: t2_1.b, ((t2_1.*)::text)
+                ->  Seq Scan on prt2_p2 t2_1
+    ->  Merge Join
+          Merge Cond: ((t1_2.a = t2_2.b) AND (((t1_2.*)::text) = ((t2_2.*)::text)))
+          ->  Sort
+                Sort Key: t1_2.a, ((t1_2.*)::text)
+                ->  Seq Scan on prt1_p3 t1_2
+          ->  Sort
+                Sort Key: t2_2.b, ((t2_2.*)::text)
+                ->  Seq Scan on prt2_p3 t2_2
+ (26 rows)
+ 
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+  a  | b  
+ ----+----
+   0 |  0
+   6 |  6
+  12 | 12
+  18 | 18
+  24 | 24
+ (5 rows)
+ 
  RESET enable_hashjoin;
  RESET enable_nestloop;
  --
*** a/src/test/regress/sql/partition_join.sql
--- b/src/test/regress/sql/partition_join.sql
***************
*** 160,165 **** EXPLAIN (COSTS OFF)
--- 160,170 ----
  SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT * FROM prt2 WHERE b > 250) t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
  SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT * FROM prt2 WHERE b > 250) t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
  
+ -- merge join when expression with whole-row reference needs to be sorted
+ EXPLAIN (COSTS OFF)
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+ 
  RESET enable_hashjoin;
  RESET enable_nestloop;

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 22 Jun 2018 19:24:37 +0530

On Tue, Jun 19, 2018 at 6:16 PM, Etsuro Fujita
<[email-masked]> wrote:
>
> * As I said upthread, the patch makes code much more simple; I removed all
> the changes to setrefs.c added by the partitionwise-join patch.  I also
> simplified the logic for building a tlist for a child-join rel. The original
> PWJ computes attr_needed data even for child rels, and build the tlist for a
> child-join by passing to build_joinrel_tlist that data for input child rels
> for the child-join.  But I think that's redundant, and it's more
> straightforward to apply adjust_appendrel_attrs to the parent-join's tlist
> to get the child-join's tlist.  So, I changed that way, which made
> unnecessary all the changes to build_joinrel_tlist and placeholder.c added
> by the PWJ patch, so I removed those as well.
>

I have started reviewing the patch.
+       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.

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().

+   /* 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.

This is not a full review. I will continue reviewing it further.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

---

From: Robert Haas <[email-masked]>
Date: Fri, 22 Jun 2018 10:58:28 -0400

> * The patch contains all of the regression tests in the original patch

I think this approach is going to run into trouble if the level at
which we have to apply the ConvertRowTypeExpr happens not to be a
projection-capable node.  And, in general, it seems to me that we want
to produce the right outputs at the lowest possible level of the plan
tree.  For instance, suppose that one of the relations being scanned
is not parallel-safe but the others are.  Then, we could end up with a
plan like this:

Append
-> Seq Scan on temp_rela
-> Gather
  -> Parallel Seq Scan on thing1
-> Gather
  -> Parallel Seq Scan on thing2

If a projection is required to convert the row type expression, we
certainly want that to get pushed below the Gather, not to happen at
the Gather level itself.  We certainly don't want it to happen at the
Append level, which can't even handle it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 29 Jun 2018 21:51:04 +0900

> 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);
> +       }
>

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).

> applicable level by level, we need to recurse in

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.

> 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

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 02 Jul 2018 18:46:35 +0900

(2018/06/22 23:58), Robert Haas wrote:
> projection-capable node.

Actually, the level we have to do that would be a child rel of a 
partitioned table or a child join of a partitionwise join, so the plan 
node would be a scan or join plan unless the child rel or child join is 
itself partitioned, in which case the plan node would be 
Append/MergeAppend and the proposed patch recursively would apply that 
conversion to child plans for the Append/MergeAppend).

> And, in general, it seems to me that we want
> the Gather level itself.

IIUC, we currently don't consider such a plan for partition-wise join; 
we'll only consider gathering partial paths for the parent appendrel. 
So, I'm not sure we need to take into account that when applying the 
ConvertRowtypeExpr.  Maybe I'm missing something, though.

> We certainly don't want it to happen at the

I think so too.

Best regards,
Etsuro Fujita

---

From: Andres Freund <[email-masked]>
Date: Tue, 3 Jul 2018 09:35:19 -0700

Hi Ashutosh, Etsuro, Robert,

> which we have to apply the ConvertRowTypeExpr happens not to be a

What's the plan forward here? This has been an open item for quite a
while. Robert, are you in agreement with this approach on a high level?
Who is going to drive this forward?

Greetings,

Andres Freund

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 04 Jul 2018 11:00:46 +0900

> What's the plan forward here?

I still think that this approach would be the right way to go, so I plan 
to polish the patch.

> This has been an open item for quite a

I'm willing to do that if it's OK, but I'd like to get more feedback 
from Robert, Ashutosh or anyone else.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 4 Jul 2018 15:34:46 +0530

On Fri, Jun 29, 2018 at 6:21 PM, Etsuro Fujita
<[email-masked]> wrote:
> 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=# explain verbose select * from (select * from lpt union all select
> * from lpt_p2) ss(c1, c2) inner join test on (ss.c1 = test.c1);

--- plan clipped

>
> 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).

FWIW, the test is not sufficient. In the above example, a simple
select * from lpt inner join test where lpt.c1 = test.c1 would not use
partition-wise join technique. Why not to avoid build_childrel_tlist()
in that case as well? Worst we can change the criteria to use
partition-wise join in future e.g. above case would use partition-wise
join by 1. merging lpt_p1 into corresponding partition of lpt so that
ss is partitioned and 2. repartitioning test or joining test with each
partition of lpt separately. In those cases the changes you are doing
here need to be reverted and put somewhere else. There's already a
patch to reverse the order of join and grouping out there. How would
this work with that.

In general I think set_append_rel_size() or build_simple_rel() is not
the place where we should decide whether the given relation will
participate in a PWJ. Also we should not selectively add a
ConvertRowtypeExpr on top of a whole-row Var of some a child relation.
We should do it for all the child rels or shouldn't do it at all. An
in-between state will produce a hell lot of confusion for any further
optimization. Whenever we change the code around partition-wise
operations in future, we will have to check whether or not a given
child rel has its whole-row Var embedded in ConvertRowtypeExpr. As I
have mentioned earlier, I am also not comfortable with the targetlist
of child relations being type inconsistent with that of the parent,
which is a fundamental rule in inheritance. Worst keep them
inconsistent during path creation and make them consistent at the time
of creating plans. A child's whole-row Var has datatype of the child
where as that of parent has datatype of parent. A ConvertRowtypeExpr
is used to fix this inconsistency. That's why I chose to use
pull_var_clause() as a place to fix the problem and not fix
ConvertRowtypeExpr in targetlist itself.

I am also not comfortable in just avoiding ConvertRowtypeExprs in
targetlist and leave them as is in the conditions and ECs etc. This
means searching a ConvertRowtypeExpr e.g. for creating pathkeys in
targetlist will fail at the time of path creation but will succeed at
the time of plan creation.

This is turning more invasive that my approach of fixing it through PVC.

> 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.

Ok. Thanks for the clarification. I think we should add a comment.

> 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.

That's not true. add_paths_to_append_rel() adds sort paths on scan if
necessary and creates merge append path.

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 04 Jul 2018 19:06:43 +0900

> we'll only consider gathering partial paths for the parent appendrel.

While updating the patch, I noticed that I'm wrong here.  IIUC, 
apply_scanjoin_target_to_paths would allow us to consider such an Append 
for a partitioned relation when scanjoin_target_parallel_safe=false, as 
it generates new Append paths by recursively adjusting all partitions 
for which we call generate_gather_paths in that case as shown below:

     /*
      * If the scan/join target is not parallel-safe, partial paths cannot
      * generate it.
      */
     if (!scanjoin_target_parallel_safe)
     {
         /*
          * Since we can't generate the final scan/join target, this is our
          * last opportunity to use any partial paths that exist.  We 
don't do
          * this if the case where the target is parallel-safe, since we 
will
          * be able to generate superior paths by doing it after the final
          * scan/join target has been applied.
          *
          * Note that this may invalidate rel->cheapest_total_path, so 
we must
          * not rely on it after this point without first calling 
set_cheapest.
          */
         generate_gather_paths(root, rel, false);

         /* Can't use parallel query above this level. */
         rel->partial_pathlist = NIL;
         rel->consider_parallel = false;
     }

I don't produce a test case where the plan is an Append with Gather 
subplans, but I'm not sure that it's a good thing to allow us to 
consider such a plan because in that plan, each Gather would try to grab 
its own pool of workers.  Am I missing something?

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 4 Jul 2018 15:41:35 +0530

On Wed, Jul 4, 2018 at 3:36 PM, Etsuro Fujita
<[email-masked]> wrote:
> subplans, but I'm not sure that it's a good thing to allow us to consider
> such a plan because in that plan, each Gather would try to grab its own pool
> of workers.  Am I missing something?

If the overall join can not use parallelism, having individual child
joins use parallel query might turn out to be efficient even if done
one child at a time. Parallel append drastically reduces the cases
where something like could be useful, but I don't think we can
theoretically eliminate the need for such a plan.

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 04 Jul 2018 21:06:11 +0900

(2018/07/04 19:04), Ashutosh Bapat wrote:
> in that case as well?

I might misunderstand your words, but in the above example the patch 
doesn't apply build_childrel_tlist to lpt_p1 and lpt_p2.  The reason for 
that is because we can avoid adjusting the tlists for the corresponding 
subplans at plan creation time so that whole-row Vars in the tlists are 
transformed into CREs.  I think the overhead of the adjustment is not 
that big, but not zero, so it would be worth avoiding applying 
build_childrel_tlist to partitions if the top parent won't participate 
in a partitionwise-join at all.

> Worst we can change the criteria to use

Interesting, but that seems like a matter of PG12+.

> We should do it for all the child rels or shouldn't do it at all.

One thing I thought was to apply build_childrel_tlist for all the child 
rels whether its top parent is a partitioned table or not, but as I 
mentioned above, that would incur needless overhead for adjusting the 
tlists for the child rels that don't involve in a partitionwise join 
such as lpt_p1 and lpt_p2, which I think is not good.

> An
> where as that of parent has datatype of parent.

I don't see any critical issue here.  Could you elaborate a bit more on 
that point?

> A ConvertRowtypeExpr

I think the biggest issue in the original patch you proposed is that we 
spend extra cycles where partitioning is not involved, which is the 
biggest reason why I think the original patch isn't the right way to go.

> This is turning more invasive that my approach of fixing it through PVC.

Sorry, I don't understand this.  Could you show me places where the 
problem happens?

> Ok. Thanks for the clarification. I think we should add a comment.

Will do.

> necessary and creates merge append path.

Really?  I think create_merge_append_path called from 
generate_mergeappend_paths called from add_paths_to_append_rel uses a 
dummy sort path just to compute the cost, but I don't think 
create_merge_append_path (or generate_mergeappend_paths or 
add_paths_to_append_rel) insert a sort path to a scan (or join) path.

Thanks for the comments!

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 4 Jul 2018 18:07:52 +0530

On Wed, Jul 4, 2018 at 5:36 PM, Etsuro Fujita
<[email-masked]> wrote:
>>> select
>>> need
> I might misunderstand your words, but in the above example the patch doesn't
> apply build_childrel_tlist to lpt_p1 and lpt_p2.  The reason for that is
> because we can avoid adjusting the tlists for the corresponding subplans at
> plan creation time so that whole-row Vars in the tlists are transformed into
> CREs.  I think the overhead of the adjustment is not that big, but not zero,
> so it would be worth avoiding applying build_childrel_tlist to partitions if
> the top parent won't participate in a partitionwise-join at all.

I don't think that answers my question. When we join lpt with test,
your patch will apply build_childrel_tlist() to lpt_p1 and lpt_p2 even
when join between lpt and test is not going to use partition-wise
join. Why? As per your explanation, the condition "if
(enable_partitionwise_join &&  rel->top_parent_is_partitioned)" is
used to avoid applying build_childrel_tlist() when partition-wise join
won't be possible. But it's not covering all the cases.

> Interesting, but that seems like a matter of PG12+.

Yes, and I don't think it's a good idea to change this fix for PG12+ again.

> One thing I thought was to apply build_childrel_tlist for all the child rels
> whether its top parent is a partitioned table or not, but as I mentioned
> above, that would incur needless overhead for adjusting the tlists for the
> child rels that don't involve in a partitionwise join such as lpt_p1 and
> lpt_p2, which I think is not good.
>
>> An
> I don't see any critical issue here.  Could you elaborate a bit more on that
> point?

I think breaking a fundamental rule like this itself is critical. But
interestingly I am not able to find a case where it becomes a problem.
But may be I haven't tried enough. And the question is if it's not
required to have the targetlists type consistent, why even bother with
ConvertRowtypeExpr addition there? We can use your approach of adding
ConvertRowtypeExpr at the end in all the cases.

> spend extra cycles where partitioning is not involved, which is the biggest
> reason why I think the original patch isn't the right way to go.

When there are no partitions involved, there won't be any
ConvertRowtypeExprs there which means the function
is_converted_whole_row_reference() would just return from the first
line checking IsA() and nullness of node. pull_var_clause() is
structured so that it always incurs IsA() cost whether or not the
corresponding type of node is present in the query or not. E.g. even
if there are no aggregates in the query, we will still incur some
cycles checking IsA(Aggref, node) when that function is called. We
could restructure the condition as IsA(node, ConvertRowtypeExpr) &&
is_converted_whole_row_reference(node) to avoid even the extra cycles
spent in function call.

> Sorry, I don't understand this.  Could you show me places where the problem
> happens?

I think breaking a fundamental rule like this itself is invasive.

> generate_mergeappend_paths called from add_paths_to_append_rel uses a dummy
> sort path just to compute the cost, but I don't think

You are right. We add sort after adjust_subplan_tlist() has been
called. So, we are fine there.

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 05 Jul 2018 20:15:42 +0900

> theoretically eliminate the need for such a plan.

In the case where scanjoin_target_parallel_safe=false, we actually will 
consider such parallel paths using the existing partial paths for the 
parent appendrel in the code path shown in a previous email (note: we 
would already have done generate_partitionwise_join_paths or 
set_append_rel_pathlist for the parent appendrel in query_planner.)  For 
such a parallel path, we might need to do a projection in the Gather 
node for generating the SRF-free scan/join target, but I think that 
would be still efficient.  So, I'm not sure that we really need to 
create child Gathers and generate an Append with those Gathers, as you 
mentioned.

Another thing I noticed is: actually, we don't produce an Append with 
child Gathers in apply_scanjoin_target_to_paths, which I thought we 
would do that in the case of scanjoin_target_parallel_safe=false, but I 
noticed I was wrong.  Sorry for that.  The reason is because in that 
case, even if we create new partial Append paths with child Gathers, we 
don't run generate_gatehr_paths for the newly created partial paths at 
the end of that function shown below, since the parent's 
consider_parallel flag is set to false in that case:

     /*
      * Consider generating Gather or Gather Merge paths.  We must only 
do this
      * if the relation is parallel safe, and we don't do it for child 
rels to
      * avoid creating multiple Gather nodes within the same plan. We 
must do
      * this after all paths have been generated and before 
set_cheapest, since
      * one of the generated paths may turn out to be the cheapest one.
      */
     if (rel->consider_parallel && !IS_OTHER_REL(rel))
         generate_gather_paths(root, rel, false);

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 05 Jul 2018 20:20:27 +0900

(2018/07/05 20:15), Etsuro Fujita wrote:
>>> consider
> would be still efficient. So, I'm not sure that we really need to create
> child Gathers and generate an Append with those Gathers, as you mentioned.
>
> noticed I was wrong. Sorry for that. The reason is because in that case,
> even if we create new partial Append paths with child Gathers, we don't
> run generate_gatehr_paths for the newly created partial paths at the end
> of that function shown below, since the parent's consider_parallel flag
> is set to false in that case:
>
> /*
> * Consider generating Gather or Gather Merge paths. We must only do this
> * if the relation is parallel safe, and we don't do it for child rels to
> * avoid creating multiple Gather nodes within the same plan. We must do
> * this after all paths have been generated and before set_cheapest, since

One thing to add is: ISTM we need some cleanup for 
apply_scanjoin_target_to_paths.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 5 Jul 2018 18:34:46 +0530

On Thu, Jul 5, 2018 at 4:45 PM, Etsuro Fujita
<[email-masked]> wrote:
> consider such parallel paths using the existing partial paths for the parent
> appendrel in the code path shown in a previous email (note: we would already
> have done generate_partitionwise_join_paths or set_append_rel_pathlist for
> the parent appendrel in query_planner.)  For such a parallel path, we might
> need to do a projection in the Gather node for generating the SRF-free
> scan/join target, but I think that would be still efficient.  So, I'm not
> sure that we really need to create child Gathers and generate an Append with
> those Gathers, as you mentioned.

I don't think it will be Append of Gathers, but Append where one of
the subplans is Gather. I don't think that possibility is completely
eliminated.

>
> Another thing I noticed is: actually, we don't produce an Append with child
> Gathers in apply_scanjoin_target_to_paths, which I thought we would do that
> in the case of scanjoin_target_parallel_safe=false, but I noticed I was
> wrong.  Sorry for that.  The reason is because in that case, even if we
> create new partial Append paths with child Gathers, we don't run
> generate_gatehr_paths for the newly created partial paths at the end of that
> function shown below, since the parent's consider_parallel flag is set to
> false in that case:
>
>     /*

Hmm. I don't think that's a great idea since such a plan might turn
out cheaper esp. when there are very few children which could use
parallel query and parallel append is possible at the top parent. But
anyway, that's what it is today. But I think, we shouldn't write code
assuming that an Append will never see a Gather below it. We might see
some plans like that in future and need to change your code.

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 06 Jul 2018 12:40:08 +0900

> parallel query and parallel append is possible at the top parent.

Actually, I don't think we can create such a Parallel Append, because in 
the case where we have scanjoin_target_parallel_safe=false, child paths 
would not be parallel-safe anymore as those paths are rewritten to have 
parallel-unsafe targets in the recursion of apply_scanjoin_target_to_paths.

> some plans like that in future and need to change your code.

I agree that we might allow such an Append in future, but I'm not sure 
we should write code for that in PG11, because 1) that would make code 
complicated than necessary and 2) that might cause overlooking of 
unexpected behavior of the planner; if the planner created such an 
Append erroneously, we would run that by that code without noticing 
that.  To reduce the maintenance burden and the overlooking risk, I 
think it's better to extend code for such a plan when we allow that plan 
for a partitionwise join.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 06 Jul 2018 19:59:54 +0900

> join. Why?

Maybe my explanation including the example was not good.  Sorry about 
that, but my patch will *not* apply build_childrel_tlist to lpt_p1 and 
lpt_p2 since the top parent of lpt_p1 and lpt_p2 is the UNION ALL 
subquery and hence not a partitioned table (ie, we have 
rel->top_parent_is_partitioned=false for lpt_p1 and lpt_p2).

> As per your explanation, the condition "if
> (enable_partitionwise_join&&   rel->top_parent_is_partitioned)" is

Let me explain about that: 1) my patch won't apply that function to a 
child if its top parent is an appendrel built from a UNION ALL subquery, 
even though the child is a partition of a partitioned table pulled up 
from a leaf subquery into the parent query, like lpt_p1, and 2) my patch 
will apply that function to a child if its top parent is a partitioned 
table, whether or not the partitioned table is involved in a 
partitionwise join.  By #1, we avoid the adjustment work at plan 
creation time, as explained above.  It might be worth trying to be 
smarter about #2 (for example, in the case of a join of a partitioned 
table and a non-partitioned table, since we don't consider a 
partitionwise join for that join, it's better to not apply that function 
to partitions of the partitioned table, to avoid the adjustment work at 
plan creation time), but ISTM we don't have enough information to be 
smarter.

> ConvertRowtypeExpr at the end in all the cases.

I think that the tlist of a (not-the-topmost) child relation doesn't 
need to be type-consistent with that of the parent; it has only to 
include all Vars that are needed for higher joinquals and final output 
to the parent appendrel.  (In other words, I think we can build the 
tlist in the same manner as we build tlists of base or join relations in 
the main join tree.)  On the other hand, other expressions such as WHERE 
quals need to be type-consistent with those of the parent; else we would 
create a plan that produces the wrong result.  So, with my patch we 
carry out special handling for the tlist; it includes a child whole-row 
Var, if needed, instead of a CRE converting the Var to the parent 
rowtype.  That allows us to create/process child-join plans using the 
existing planner functions as-is.

> line checking IsA() and nullness of node.

Really?  IIRC, with the original patch we would spend extra cycles in a 
non-partitioned inheritance processing [1].

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5AFC0865.8050802%40lab.ntt.co.jp

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 6 Jul 2018 16:50:39 +0530

On Fri, Jul 6, 2018 at 4:29 PM, Etsuro Fujita
<[email-masked]> wrote:
>>>>> build_childrel_tlist
>>>>> to
>>> doesn't
> Maybe my explanation including the example was not good.  Sorry about that,
> but my patch will *not* apply build_childrel_tlist to lpt_p1 and lpt_p2
> since the top parent of lpt_p1 and lpt_p2 is the UNION ALL subquery and
> hence not a partitioned table (ie, we have
> Let me explain about that: 1) my patch won't apply that function to a child
> if its top parent is an appendrel built from a UNION ALL subquery, even
> though the child is a partition of a partitioned table pulled up from a leaf
> subquery into the parent query, like lpt_p1, and 2) my patch will apply that
> function to a child if its top parent is a partitioned table, whether or not
> the partitioned table is involved in a partitionwise join.  By #1, we avoid
> the adjustment work at plan creation time, as explained above.  It might be
> worth trying to be smarter about #2 (for example, in the case of a join of a
> partitioned table and a non-partitioned table, since we don't consider a
> partitionwise join for that join, it's better to not apply that function to
> partitions of the partitioned table, to avoid the adjustment work at plan
> creation time), but ISTM we don't have enough information to be smarter.

That looks like a kludge to me rather than a proper fix. It's not
clear to me as to when somebody can expect ConvertRowtypeExpr in the
targetlist and when don't while creating paths and to an extent plans.
For example, inside add_paths_to_append_rel() or in
apply_scanjoin_target_to_paths() or for that matter any path creation
or plan creation function, we will sometimes get targetlists with
ConvertRowtypeExpr() and sometime not. How do we know which is when.

>>> biggest

As I said, we do spend cycles in that function testing whether a node
is Aggref or not even when the query doesn't have aggregates or
grouping OR spend cycles in testing whether a node is a PlaceHolderVar
when the query doesn't produce any. So, I don't see any problem with
spending a few cycles testing whether a node is ConvertRowtypeExpr or
not when a ConvertRowtypeExpr is not in the query or command. That's
not a huge performance trouble. I would be happy to change my mind, if
you show me performance different with and without this patch in
planning. I haven't seen any.

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 09 Jul 2018 20:03:05 +0900

> ConvertRowtypeExpr() and sometime not. How do we know which is when.

That is known from a new flag "has_child_wholerow" added to RelOptInfo 
to indicate whether an other relation's reltarget has child wholerow 
Vars instead of ConvertRowtypeExprs.  That flag is initialized to false 
and only set to true in build_childrel_tlist if it adds child wholerow 
Vars to the reltarget of a given other relation.  Though, I don't think 
we need to be conscious about whether the reltarget has child wholerow 
Vars or ConvertRowtypeExprs when creating paths; we would just create 
paths based on the reltarget.  What we need to be careful about is when 
creating an Append/MergeAppend plan; we have to adjust the subplan's 
tlist so child wholerow Vars in that tlist are converted to the 
corresponding ConvertRowtypeExprs, if that tlist has those Vars, which 
is also known from the new flag.

> planning. I haven't seen any.

I have to admit that the case in [1] wouldn't affect the performance, 
but my concern is that there might be some cases where the test affects 
performance.  In contrast, in the approach I proposed, we wouldn't need 
to worry about that because the approach does not modify code that is 
not related to partitioning.  I think that's a good thing.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 9 Jul 2018 16:36:02 +0530

On Mon, Jul 9, 2018 at 4:33 PM, Etsuro Fujita
<[email-masked]> wrote:
> I have to admit that the case in [1] wouldn't affect the performance, but my
> concern is that there might be some cases where the test affects
> performance.

What are those cases? Can you please provide any numbers supporting your claim?

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 09 Jul 2018 20:35:34 +0900

> What are those cases? Can you please provide any numbers supporting your claim?

I don't have any numbers right now, so that is nothing but a concern. 
But as I said in a previous email, in the approach I proposed, we don't 
need to spend extra cycles where partitioning is not involved.  I think 
that is a good thing in itself.  No?

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 9 Jul 2018 17:13:09 +0530

>
> I don't have any numbers right now, so that is nothing but a concern. But as
> I said in a previous email, in the approach I proposed, we don't need to
> spend extra cycles where partitioning is not involved.  I think that is a
> good thing in itself.  No?

At the cost of having targetlist being type inconsistent. I don't have
any testcase either to show that that's a problem in practice. So,
it's a trade-off of a concern vs concern.

Apart from that, in your approach there are extra cycles spent in
traversing the targetlist to add ConvertRowtypeExpr, albeit only when
there is a whole-row expression in the targetlist, when creating
plans. That's not there in my patch.

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 10 Jul 2018 12:38:40 +0900

> it's a trade-off of a concern vs concern.

As I said before, I don't see any issue in having such a targetlist, but 
I might be missing something, so I'd like to discuss a bit more about 
that.  Could you tell me the logic/place in the PG code where you think 
the problem might occur.  (IIRC, you mentioned something about that 
before (pathkeys? or index-only scans?), but sorry, I don't understand 
that.)

> plans. That's not there in my patch.

Right.  That's unfortunate, but I think that that would be still better 
than needing to spent extra cycles where partitioning is not involved. 
And ISTM that the traversing cost is not that large compared to the cost 
we pay before that for query planning for a partitionwise join.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 10 Jul 2018 16:28:13 +0530

On Tue, Jul 10, 2018 at 9:08 AM, Etsuro Fujita
<[email-masked]> wrote:
> As I said before, I don't see any issue in having such a targetlist, but I
> might be missing something, so I'd like to discuss a bit more about that.
> Could you tell me the logic/place in the PG code where you think the problem
> might occur.  (IIRC, you mentioned something about that before (pathkeys? or
> index-only scans?), but sorry, I don't understand that.)

IIUC, index-only scan will be used when the all the required columns
are covered by an index. If there is an index on the whole-row
reference of the parent, it will be translated into a
ConvertRowtypeExpr of the child when an index on the child is created.
If the targetlist doesn't have ConvertRowtypeExpr, as your patch does,
the planner won't be able to use such an index on the child table. But
I couldn't create an index with a whole-row reference in it. So, I
think this isn't possible right now. But in future if we allow
creating index on whole-row reference or

Pathkey points to an equivalence class, which contains equivalence
members. A parent equivalence class member containing a whole-row
reference gets translated into a child equivalence member containing a
ConvertRowtypeExpr. At places in planner we match equivalence members
to the targetlist entries. This matching will fail unexpectedly when
ConvertRowtypeExpr is removed from a child's targetlist. But again I
couldn't reproduce a problem when such a mismatch arises.

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 11 Jul 2018 16:53:27 +0900

(2018/07/10 19:58), Ashutosh Bapat wrote:
>>> any testcase either to show that that's a problem in practice.

I think so too.

> think this isn't possible right now.

Actually, even if we could create such an index on the child table and 
the targetlist had the ConvertRowtypeExpr, the planner would still not 
be able to use an index-only scan with that index; because 
check_index_only would not consider that an index-only scan is possible 
for that index because that index is an expression index and that 
function currently does not consider that index expressions are able to 
be returned back in an index-only scan.  That behavior of the planner 
might be improved in future, though.

I think so too.

> At places in planner we match equivalence members

IIUC, I don't think the planner assumes that for an equivalence member 
there is an matching entry for that member in the targetlist; what I 
think the planner assumes is: an equivalence member is able to be 
computed from expressions in the targetlist.  So, I think it is safe to 
have whole-row Vars instead of ConvertRowtypeExprs in the targetlist.

Thanks for the explanation!

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 11 Jul 2018 16:32:50 +0530

On Wed, Jul 11, 2018 at 1:23 PM, Etsuro Fujita
<[email-masked]> wrote:
>
>
> Actually, even if we could create such an index on the child table and the
> targetlist had the ConvertRowtypeExpr, the planner would still not be able
> to use an index-only scan with that index; because check_index_only would
> not consider that an index-only scan is possible for that index because that
> index is an expression index and that function currently does not consider
> that index expressions are able to be returned back in an index-only scan.
> That behavior of the planner might be improved in future, though.
>

Right and when we do so, not having ConvertRowtypeExpr in the
targetlist will be a problem.

> IIUC, I don't think the planner assumes that for an equivalence member there
> is an matching entry for that member in the targetlist; what I think the
> planner assumes is: an equivalence member is able to be computed from
> expressions in the targetlist.

This is true. However,

>  So, I think it is safe to have whole-row
> Vars instead of ConvertRowtypeExprs in the targetlist.

when it's looking for an expression, it finds a whole-row expression
so it think it needs to add a ConvertRowtypeExpr on that. But when the
plan is created, there is ConvertRowtypeExpr already, but there is no
way to know that a new ConvertRowtypeExpr is not needed anymore. So,
we may have two ConvertRowtypeExprs giving wrong results.

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 12 Jul 2018 12:32:03 +0900

> targetlist will be a problem.

Yeah, but I don't think that that's unsolvable; because in that case the 
CRE as an index expression could be converted back to the whole-row 
Var's rowtype by adding another CRE to the index expression for that 
conversion, I suspect that that special handling could allow us to 
support an index-only scan even when having the whole-row Var instead of 
the CRE in the targetlist.  (Having said that, I'm not 100% sure we need 
to solve that problem when we improve the planner, because there doesn't 
seem to me to be enough use-case to justify making the code complicated 
for that.)  Anyway, I think that that would be a matter of future 
versions of PG.

> we may have two ConvertRowtypeExprs giving wrong results.

Maybe I'm missing something, but I don't think that we need to worry 
about that, because in the approach I proposed, we only add CREs above 
whole-row Vars in the targetlists for subplans of an Append/MergeAppend 
for a partitioned relation at plan creation time.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 12 Jul 2018 10:08:13 +0530

On Thu, Jul 12, 2018 at 9:02 AM, Etsuro Fujita
<[email-masked]> wrote:
>>> the
> Yeah, but I don't think that that's unsolvable; because in that case the CRE
> as an index expression could be converted back to the whole-row Var's
> rowtype by adding another CRE to the index expression for that conversion, I
> suspect that that special handling could allow us to support an index-only
> scan even when having the whole-row Var instead of the CRE in the
> targetlist.

I am not able to understand this. Can you please provide an example?

> (Having said that, I'm not 100% sure we need to solve that
> problem when we improve the planner, because there doesn't seem to me to be
> enough use-case to justify making the code complicated for that.)  Anyway, I
> think that that would be a matter of future versions of PG.

I am afraid that a fix which just works only till we change the other
parts of the system is useful only till that time. At that time, it
needs to be replaced with a different fix. If that time is long
enough, that's ok. In this case, I agree that if we haven't heard from
users till now that they need to create indexes on whole-row
expressions, there's chance that we will never implement this feature.

>>> there
> Maybe I'm missing something, but I don't think that we need to worry about
> that, because in the approach I proposed, we only add CREs above whole-row
> Vars in the targetlists for subplans of an Append/MergeAppend for a
> partitioned relation at plan creation time.

There's a patch in an adjacent thread started by David Rowley to rip
out Append/MergeAppend when there is only one subplan. So, your
solution won't work there.

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 12 Jul 2018 20:02:05 +0900

> I am not able to understand this. Can you please provide an example?

Sorry, my explanation was not good.  Let me try.  We can't create an 
inherited index on the whole-row reference in a partitioned table, but 
actually, we can directly create the corresponding index on a child table:

postgres=# create table pt (a int, b text) partition by list (a);
CREATE TABLE
postgres=# create table ptp1 (b text, a int check (a in (1)));
CREATE TABLE
postgres=# alter table pt attach partition ptp1 for values in (1);
ALTER TABLE
postgres=# create index ptp1_conv_wr_idx on ptp1 ((ptp1::pt));
CREATE INDEX
postgres=# insert into pt values (1, 'foo');
INSERT 0 1
postgres=# select *, ptp1 as wr, ptp1::pt as conv_wr from ptp1;
   b  | a |   wr    | conv_wr
-----+---+---------+---------
  foo | 1 | (foo,1) | (1,foo)
(1 row)

In this example, the value of the whole-row reference to the child table 
ptp1 for that record is ('foo',1), and that of the index expression for 
that record is (1,'foo').  Those have different column orders, but the 
latter could be mapped to the former by a technique like 
do_convert_tuple.  So, what I suspect is: by producing the value of the 
whole-row reference from that of the index expression like that, we 
could support index-only scans with such an index in the case where we 
have the whole-row reference in the targetlist, not the index expression 
itself.

> needs to be replaced with a different fix.

I agree on that point.

> If that time is long

I don't object to adding that feature to future versions of PG if required.

> solution won't work there.

Thanks for sharing that information!  I skimmed the thread.  I haven't 
yet caught up with all the discussions there, so I think I'm missing 
something, but it looks like that we haven't yet reached any consensus 
on the way to go.  In my opinion, I like the approach mentioned in [1]. 
  And if we go that way, my patch seems to fit into that, because in 
that approach the Append/MergeAppend could be removed after adjusting 
the targetlists for its subplans in 
create_append_plan/create_merge_append_plan.  Anyway, I'd like to join 
in that work for PG12.

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/1867.1509032831%40sss.pgh.pa.us

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 13 Jul 2018 19:35:05 +0530

On Thu, Jul 12, 2018 at 4:32 PM, Etsuro Fujita
<[email-masked]> wrote:
> ptp1 for that record is ('foo',1), and that of the index expression for that
> record is (1,'foo').  Those have different column orders, but the latter
> could be mapped to the former by a technique like do_convert_tuple.  So,
> what I suspect is: by producing the value of the whole-row reference from
> that of the index expression like that,

The expression in this case would look like ptp1::pt::ptp1 which won't
match targetlist expression ptp1. I am also doubtful that the planner
will be able to deduce that it need to apply an inverse function of
::pt and what exactly such an inverse function is. So index only scan
won't be picked.

> we could support index-only scans
> with such an index in the case where we have the whole-row reference in the
> targetlist, not the index expression itself.

Can you please show an index only scan path being created in this case?

>>> about
> Thanks for sharing that information!  I skimmed the thread.  I haven't yet
> caught up with all the discussions there, so I think I'm missing something,
> but it looks like that we haven't yet reached any consensus on the way to
> go.  In my opinion, I like the approach mentioned in [1].  And if we go that
> way, my patch seems to fit into that, because in that approach the
> Append/MergeAppend could be removed after adjusting the targetlists for its
> subplans in create_append_plan/create_merge_append_plan.  Anyway, I'd like
> to join in that work for PG12.

Whatever may be the outcome of that work, I think what we fix here
shouldn't require to reverted in a few months from now, just so that
that patch works.

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 18 Jul 2018 21:00:41 +0900

> I still think that this approach would be the right way to go, so I plan

The approach would not require extra cycles where partitioning is not 
involved as discussed before, and makes code much simpler, which would 
make maintenance and the FDW author's life easy, so I still think that 
the approach would be the direction to go in.  So, I updated the patch. 
  Here are changes:

* I assumed that the child plan passed to adjust_subplan_tlist was a 
scan or join unless it was Append or MergeAppend, but I noticed I was 
wrong; if the topmost scan/join relation is partitioned, 
apply_scanjoin_target_to_paths might have pushed down ProjectSet or 
Result to each of partitions.  So, in that case the child's plan would 
have ProjectSet or Result atop the scan or join plan.  (I could not 
produce such an example, though.)  To address this, I modified 
build_childrel_tlist so that we just apply adjust_appendrel_attrs to the 
parent's targetlist to get the child's if the parent is the topmost 
scan/join relation.  So, we don't need to care about that in 
adjust_subplan_tlist.  I think this change would also increase the 
efficiency for that case.  I added regression test cases for that as well.

* I fixed an oversight in outfuncs.c.

* I renamed member variables I added to RelOptInfo before, and revised 
code/comments a bit.

Attached is an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/postgres-master/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/postgres-master/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 8337,8344 **** ALTER TABLE fprt2_p1 SET (autovacuum_enabled = 'false');
  ALTER TABLE fprt2_p2 SET (autovacuum_enabled = 'false');
  INSERT INTO fprt2_p1 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(0, 249, 3) i;
  INSERT INTO fprt2_p2 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(250, 499, 3) i;
! CREATE FOREIGN TABLE ftprt2_p1 PARTITION OF fprt2 FOR VALUES FROM (0) TO (250)
  	SERVER loopback OPTIONS (table_name 'fprt2_p1', use_remote_estimate 'true');
  CREATE FOREIGN TABLE ftprt2_p2 PARTITION OF fprt2 FOR VALUES FROM (250) TO (500)
  	SERVER loopback OPTIONS (table_name 'fprt2_p2', use_remote_estimate 'true');
  ANALYZE fprt2;
--- 8337,8345 ----
  ALTER TABLE fprt2_p2 SET (autovacuum_enabled = 'false');
  INSERT INTO fprt2_p1 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(0, 249, 3) i;
  INSERT INTO fprt2_p2 SELECT i, i, to_char(i/50, 'FM0000') FROM generate_series(250, 499, 3) i;
! CREATE FOREIGN TABLE ftprt2_p1 (b int, c varchar, a int)
  	SERVER loopback OPTIONS (table_name 'fprt2_p1', use_remote_estimate 'true');
+ ALTER TABLE fprt2 ATTACH PARTITION ftprt2_p1 FOR VALUES FROM (0) TO (250);
  CREATE FOREIGN TABLE ftprt2_p2 PARTITION OF fprt2 FOR VALUES FROM (250) TO (500)
  	SERVER loopback OPTIONS (table_name 'fprt2_p2', use_remote_estimate 'true');
  ANALYZE fprt2;
***************
*** 8391,8416 **** SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * FROM fprt2 WHERE a < 10)

  
--- 8392,8427 ----

  
  -- join with lateral reference
  EXPLAIN (COSTS OFF)
***************
*** 8474,8479 **** SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE
--- 8485,8520 ----
       |        | 475 | t2_phv
  (14 rows)
  
+ -- test FOR UPDATE
+ EXPLAIN (COSTS OFF)
*** a/postgres-master/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/postgres-master/contrib/postgres_fdw/sql/postgres_fdw.sql
*** 2263,2270 **** ALTER TABLE fprt2_p1 SET (autovacuum_enabled = 'false');
--- 2263,2271 ----
*** 2283,2290 **** SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * FROM fprt2 WHERE a < 10)

--- 2284,2291 ----

  -- join with lateral reference
  EXPLAIN (COSTS OFF)
***************
*** 2296,2301 **** EXPLAIN (COSTS OFF)
--- 2297,2307 ----
  SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
  SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
  
+ -- test FOR UPDATE
+ EXPLAIN (COSTS OFF)

*** a/postgres-master/src/backend/nodes/outfuncs.c
--- b/postgres-master/src/backend/nodes/outfuncs.c
***************
*** 2357,2363 **** _outRelOptInfo(StringInfo str, const RelOptInfo *node)
--- 2357,2367 ----
  	WRITE_UINT_FIELD(baserestrict_min_security);
  	WRITE_NODE_FIELD(joininfo);
  	WRITE_BOOL_FIELD(has_eclass_joins);
+ 	WRITE_OID_FIELD(top_parent_rowtype);
  	WRITE_BITMAPSET_FIELD(top_parent_relids);
+ 	WRITE_BOOL_FIELD(top_parent_parted);
+ 	WRITE_BOOL_FIELD(top_parent_topmost);
+ 	WRITE_BOOL_FIELD(need_adjust_tlist);
  	WRITE_NODE_FIELD(partitioned_child_rels);
  }
  
*** a/postgres-master/src/backend/optimizer/path/allpaths.c
--- b/postgres-master/src/backend/optimizer/path/allpaths.c
***************
*** 909,914 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
--- 909,917 ----
  		did_pruning = true;
  	}
  
+ 	if (rel->reloptkind == RELOPT_BASEREL)
+ 		rel->top_parent_topmost = bms_equal(rel->relids, root->all_baserels);
+ 
  	/*
  	 * Initialize to compute size estimates for whole append relation.
  	 *
***************
*** 956,1025 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  		childrel = find_base_rel(root, childRTindex);
  		Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL);
  
- 		if (rel->part_scheme)
- 		{
- 			AttrNumber	attno;
- 
- 			/*
- 			 * We need attr_needed data for building targetlist of a join
- 			 * relation representing join between matching partitions for
- 			 * partitionwise join. A given attribute of a child will be needed
- 			 * in the same highest joinrel where the corresponding attribute
- 			 * of parent is needed. Hence it suffices to use the same Relids
- 			 * set for parent and child.
- 			 */
- 			for (attno = rel->min_attr; attno <= rel->max_attr; attno++)
- 			{
- 				int			index = attno - rel->min_attr;
- 				Relids		attr_needed = rel->attr_needed[index];
- 
- 				/* System attributes do not need translation. */
- 				if (attno <= 0)
- 				{
- 					Assert(rel->min_attr == childrel->min_attr);
- 					childrel->attr_needed[index] = attr_needed;
- 				}
- 				else
- 				{
- 					Var		   *var = list_nth_node(Var,
- 													appinfo->translated_vars,
- 													attno - 1);
- 					int			child_index;
- 
- 					/*
- 					 * Ignore any column dropped from the parent.
- 					 * Corresponding Var won't have any translation. It won't
- 					 * have attr_needed information, since it can not be
- 					 * referenced in the query.
- 					 */
- 					if (var == NULL)
- 					{
- 						Assert(attr_needed == NULL);
- 						continue;
- 					}
- 
- 					child_index = var->varattno - childrel->min_attr;
- 					childrel->attr_needed[child_index] = attr_needed;
- 				}
- 			}
- 		}
- 
  		/*
! 		 * Copy/Modify targetlist. Even if this child is deemed empty, we need
! 		 * its targetlist in case it falls on nullable side in a child-join
! 		 * because of partitionwise join.
  		 *
! 		 * NB: the resulting childrel->reltarget->exprs may contain arbitrary
! 		 * expressions, which otherwise would not occur in a rel's targetlist.
! 		 * Code that might be looking at an appendrel child must cope with
! 		 * such.  (Normally, a rel's targetlist would only include Vars and
! 		 * PlaceHolderVars.)  XXX we do not bother to update the cost or width
! 		 * fields of childrel->reltarget; not clear if that would be useful.
  		 */
! 		childrel->reltarget->exprs = (List *)
! 			adjust_appendrel_attrs(root,
! 								   (Node *) rel->reltarget->exprs,
! 								   1, &appinfo);
  
  		/*
--- 959,974 ----
  
  		/*
! 		 * We have to copy the parent's targetlist to the child, with
! 		 * appropriate substitution of variables.
  		 *
! 		 * Note: when considering partitionwise joining, we need the child's
! 		 * targetlist in case it falls on nullable side in a child join even
! 		 * if the child is deemed empty.
  		 */
! 		childrel->top_parent_topmost = rel->top_parent_topmost;
! 		build_childrel_tlist(root, rel, childrel, 1, &appinfo);
  
  		/*
--- 1130,1143 ----
  								   1, &appinfo);
  
  		/*
*** a/postgres-master/src/backend/optimizer/plan/createplan.c
--- b/postgres-master/src/backend/optimizer/plan/createplan.c
*** 162,167 **** static void copy_generic_path_info(Plan *dest, Path *src);
--- 163,169 ----
*** 1065,1070 **** create_append_plan(PlannerInfo *root, AppendPath *best_path)
--- 1067,1076 ----
  
+ 		/* Adjust the plan's targetlist if necessary */
+ 		if (subpath->parent->need_adjust_tlist)
+ 			adjust_subplan_tlist(root, subpath->parent, subplan);
+ 
  		subplans = lappend(subplans, subplan);
  	}
  
***************
*** 1176,1181 **** create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path)
--- 1182,1191 ----
  		/* Must insist that all children return the same tlist */
  		subplan = create_plan_recurse(root, subpath, CP_EXACT_TLIST);
  
+ 		/* Adjust the plan's targetlist if necessary */
+ 		if (subpath->parent->need_adjust_tlist)
+ 			adjust_subplan_tlist(root, subpath->parent, subplan);
+ 
  		/* Compute sort column info, and adjust subplan's tlist as needed */
  		subplan = prepare_sort_from_pathkeys(subplan, pathkeys,
  											 subpath->parent->relids,
***************
*** 4968,4973 **** bitmap_subplan_mark_shared(Plan *plan)
--- 4978,5045 ----
  		elog(ERROR, "unrecognized node type: %d", nodeTag(plan));
  }
  
+ /*
+  * adjust_subplan_tlist
+  *	  If the child plan's targetlist contains a whole-row Var as an entry of
+  *	  that list, add a ConvertRowtypeExpr above the Var to convert the tuple
+  *	  layout of the Var back to the top parent's rowtype.
+  */
+ static void
+ adjust_subplan_tlist(PlannerInfo *root, RelOptInfo *rel, Plan *subplan)
+ {
+ 	List	   *newtlist;
+ 	ListCell   *l;
+ 
+ 	/* The top parent appendrel should be partitioned */
+ 	Assert(rel->top_parent_parted);
+ 	/* But it should not be the topmost scan/join relation */
+ 	Assert(!rel->top_parent_topmost);
+ 
+ 	/* The given rel should represent a scan/join relation */
+ 	Assert(rel->reloptkind == RELOPT_OTHER_MEMBER_REL ||
+ 		   rel->reloptkind == RELOPT_OTHER_JOINREL);
+ 
+ 	/*
+ 	 * If the child plan is an Append or MergeAppend, the targetlists of its
+ 	 * subplans would have already been adjusted before we get here, so need
+ 	 * to work here.
+ 	 */
+ 	if (IsA(subplan, Append) || IsA(subplan, MergeAppend))
+ 		return;
+ 
+ 	/* The child plan should be a scan or join */
+ 	Assert(is_projection_capable_plan(subplan));
+ 
+ 	newtlist = NIL;
+ 	foreach(l, subplan->targetlist)
+ 	{
+ 		TargetEntry *tle = (TargetEntry *) lfirst(l);
+ 		Var		   *var = (Var *) tle->expr;
+ 
+ 		if (IsA(var, Var) && var->varattno == 0)
+ 		{
+ 			RelOptInfo *partrel = find_base_rel(root, var->varno);
+ 			ConvertRowtypeExpr *cre = makeNode(ConvertRowtypeExpr);
+ 
+ 			/* The rel should be a partition */
+ 			Assert(partrel->reloptkind == RELOPT_OTHER_MEMBER_REL);
+ 
+ 			/* The top parent rel should be a partitioned table */
+ 			Assert(OidIsValid(partrel->top_parent_rowtype));
+ 
+ 			cre->arg = (Expr *) var;
+ 			cre->resulttype = partrel->top_parent_rowtype;
+ 			cre->convertformat = COERCE_IMPLICIT_CAST;
+ 			cre->location = -1;
+ 
+ 			tle = flatCopyTargetEntry(tle);
+ 			tle->expr = (Expr *) cre;
+ 		}
+ 		newtlist = lappend(newtlist, tle);
+ 	}
+ 	subplan->targetlist = newtlist;
+ }
+ 
  /*****************************************************************************
   *
   *	PLAN NODE BUILDING ROUTINES
*** a/postgres-master/src/backend/optimizer/plan/setrefs.c
--- b/postgres-master/src/backend/optimizer/plan/setrefs.c
***************
*** 41,49 **** typedef struct
  	int			num_vars;		/* number of plain Var tlist entries */
  	bool		has_ph_vars;	/* are there PlaceHolderVar entries? */
  	bool		has_non_vars;	/* are there other entries? */
- 	bool		has_conv_whole_rows;	/* are there ConvertRowtypeExpr
- 										 * entries encapsulating a whole-row
- 										 * Var? */
  	tlist_vinfo vars[FLEXIBLE_ARRAY_MEMBER];	/* has num_vars entries */
  } indexed_tlist;
  
--- 41,46 ----
***************

  
  
  	if (node == NULL)
  		return NULL;
--- 2245,2250 ----
***************
*** a/postgres-master/src/backend/optimizer/util/placeholder.c
--- b/postgres-master/src/backend/optimizer/util/placeholder.c
  
--- 20,25 ----
***************
  
*** a/postgres-master/src/backend/optimizer/util/relnode.c
--- b/postgres-master/src/backend/optimizer/util/relnode.c
***************
*** 16,21 ****
--- 16,22 ----
  
  #include <limits.h>

  /*
***************
*** 188,193 **** build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
--- 195,205 ----
  	rel->baserestrict_min_security = UINT_MAX;
  	rel->joininfo = NIL;
  	rel->has_eclass_joins = false;
+ 	rel->top_parent_rowtype = InvalidOid;
+ 	rel->top_parent_relids = NULL;
+ 	rel->top_parent_parted = false;
+ 	rel->top_parent_topmost = false;
+ 	rel->need_adjust_tlist = false;
  	rel->part_scheme = NULL;
  	rel->nparts = 0;
  	rel->boundinfo = NULL;
***************
*** 201,207 **** build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
  	 * Pass top parent's relids down the inheritance hierarchy. If the parent
  	 * has top_parent_relids set, it's a direct or an indirect child of the
  	 * top parent indicated by top_parent_relids. By extension this child is
! 	 * also an indirect child of that parent.
  	 */
  	if (parent)
  	{
--- 213,221 ----
  	 * Pass top parent's relids down the inheritance hierarchy. If the parent
  	 * has top_parent_relids set, it's a direct or an indirect child of the
  	 * top parent indicated by top_parent_relids. By extension this child is
! 	 * also an indirect child of that parent.  Likeweise for the flag to
! 	 * indicate whether that parent is a partitioned table and the OID of that
! 	 * parent's rowtype.
  	 */
  	if (parent)
  	{
***************
*** 209,218 **** build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
--- 223,243 ----
  			rel->top_parent_relids = parent->top_parent_relids;
  		else
  			rel->top_parent_relids = bms_copy(parent->relids);
+ 
+ 		rel->top_parent_parted = parent->top_parent_parted;
+ 		rel->top_parent_rowtype = parent->top_parent_rowtype;
  	}
  	else
+ 	{
  		rel->top_parent_relids = NULL;
  
+ 		if (rte->relkind == RELKIND_PARTITIONED_TABLE)
+ 		{
+ 			rel->top_parent_parted = true;
+ 			rel->top_parent_rowtype = get_rel_type_id(rte->relid);
+ 		}
+ 	}
+ 
  	/* Check type of rtable entry */
  	switch (rte->rtekind)
  	{
***************
*** 602,608 **** build_join_rel(PlannerInfo *root,
--- 627,637 ----
  	joinrel->baserestrict_min_security = UINT_MAX;
  	joinrel->joininfo = NIL;
  	joinrel->has_eclass_joins = false;
+ 	joinrel->top_parent_rowtype = InvalidOid;
  	joinrel->top_parent_relids = NULL;
+ 	joinrel->top_parent_parted = false;
+ 	joinrel->top_parent_topmost = false;
+ 	joinrel->need_adjust_tlist = false;
  	joinrel->part_scheme = NULL;
  	joinrel->nparts = 0;
  	joinrel->boundinfo = NULL;
***************
*** 659,664 **** build_join_rel(PlannerInfo *root,
--- 688,699 ----
  	/* Store the partition information. */
  	build_joinrel_partition_info(joinrel, outer_rel, inner_rel, restrictlist,
  								 sjinfo->jointype);
+ 	if (IS_PARTITIONED_REL(joinrel))
+ 	{
+ 		joinrel->top_parent_parted = true;
+ 		joinrel->top_parent_topmost = bms_equal(joinrel->relids,
+ 												root->all_baserels);
+ 	}
  
  	/*
  	 * Set estimates of the joinrel's size.
***************
*** 773,779 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
--- 808,818 ----
  	joinrel->baserestrictcost.per_tuple = 0;
  	joinrel->joininfo = NIL;
  	joinrel->has_eclass_joins = false;
+ 	joinrel->top_parent_rowtype = InvalidOid;
  	joinrel->top_parent_relids = NULL;
+ 	joinrel->top_parent_parted = parent_joinrel->top_parent_parted;
+ 	joinrel->top_parent_topmost = parent_joinrel->top_parent_topmost;
+ 	joinrel->need_adjust_tlist = false;
  	joinrel->part_scheme = NULL;
  	joinrel->nparts = 0;
  	joinrel->boundinfo = NULL;
***************
*** 785,802 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
  
  	joinrel->top_parent_relids = bms_union(outer_rel->top_parent_relids,
  										   inner_rel->top_parent_relids);

! 	/* Build targetlist */
  
--- 824,841 ----
  
  	joinrel->top_parent_relids = bms_union(outer_rel->top_parent_relids,
  										   inner_rel->top_parent_relids);
+ 	Assert(joinrel->top_parent_parted);

  
*** 895,909 **** static void
  
--- 934,942 ----
  
  	foreach(vars, input_rel->reltarget->exprs)
  	{
  		Var		   *var = (Var *) lfirst(vars);
***************
*** 919,972 **** build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
  
  		/*

--- 952,974 ----
  
  		/*

*** 1215,1220 **** fetch_upper_rel(PlannerInfo *root, UpperRelationKind kind, Relids relids)
--- 1217,1224 ----
  
+ 	upperrel->need_adjust_tlist = false;
+ 
  	root->upper_rels[kind] = lappend(root->upper_rels[kind], upperrel);
  
  	return upperrel;
***************
*** 1768,1770 **** build_joinrel_partition_info(RelOptInfo *joinrel, RelOptInfo *outer_rel,
--- 1772,1884 ----
  		joinrel->nullable_partexprs[cnt] = nullable_partexpr;
  	}
  }
+ 
+ /*
+  * build_childrel_tlist
+  *	  Build the child's targetlist from the parent's with
+  *	  adjust_appendrel_attrs
+  *
+  * When considering partitionwise joining, we need special handling: if the
+  * entry in the parent's targetlist is a whole-row Var for the parent rel,
+  * create a new whole-row Var for the child rel and add it to the child's
+  * targetlist.  This allows us to handle the child's targetlist in the same
+  * way as the parent's during planning.  The tuple layout of the child Var
+  * will be converted back to the parent's rowtype at create_plan time.
+  */
+ void
+ build_childrel_tlist(PlannerInfo *root,
+ 					 RelOptInfo *parentrel,
+ 					 RelOptInfo *childrel,
+ 					 int nappinfos,
+ 					 AppendRelInfo **appinfos)
+ {
+ 	ListCell   *lc;
+ 
+ 	/*
+ 	 * If enable_partitionwise_join=off, if the parent is not partitioned, or
+ 	 * if the parent is the topmost scan/join relation, we don't consider
+ 	 * partitionwise joining, so no need for special handling; just apply
+ 	 * adjust_appendrel_attrs to the parent's targetlist to get the child's.
+ 	 *
+ 	 * NB: the resulting childrel->reltarget->exprs may contain arbitrary
+ 	 * expressions, which otherwise would not occur in a rel's targetlist.
+ 	 * Code that might be looking at an appendrel child must cope with such.
+ 	 * (Normally, a rel's targetlist would only include Vars and
+ 	 * PlaceHolderVars.)  XXX we do not bother to update the cost or width
+ 	 * fields of childrel->reltarget; not clear if that would be useful.
+ 	 */
+ 	if (!enable_partitionwise_join ||
+ 		!childrel->top_parent_parted ||
+ 		childrel->top_parent_topmost)
+ 	{
+ 		childrel->reltarget->exprs = (List *)
+ 			adjust_appendrel_attrs(root,
+ 								   (Node *) parentrel->reltarget->exprs,
+ 								   nappinfos, appinfos);
+ 		return;
+ 	}
+ 
+ 	foreach(lc, parentrel->reltarget->exprs)
+ 	{
+ 		Node	   *node = (Node *) lfirst(lc);
+ 
+ 		/* Should be a simple Var or PlaceHolderVar ... */
+ 		Assert(IsA(node, Var) || IsA(node, PlaceHolderVar));
+ 
+ 		if (IsA(node, Var) && ((Var *) node)->varattno == 0)
+ 		{
+ 			Var		   *var = (Var *) copyObject(node);
+ 			AppendRelInfo *appinfo = NULL;
+ 			int			cnt;
+ 
+ 			/* Find the AppendRelInfo associated with the parent */
+ 			for (cnt = 0; cnt < nappinfos; cnt++)
+ 			{
+ 				if (var->varno == appinfos[cnt]->parent_relid)
+ 				{
+ 					appinfo = appinfos[cnt];
+ 					break;
+ 				}
+ 			}
+ 
+ 			/* Should have found the entry ... */
+ 			Assert(appinfo);
+ 
+ 			var->varno = appinfo->child_relid;
+ 			var->varnoold = appinfo->child_relid;
+ 			var->vartype = appinfo->child_reltype;
+ 
+ 			childrel->reltarget->exprs =
+ 				lappend(childrel->reltarget->exprs, var);
+ 			childrel->need_adjust_tlist = true;
+ 		}
+ 		else
+ 		{
+ 			childrel->reltarget->exprs =
+ 				lappend(childrel->reltarget->exprs,
+ 						adjust_appendrel_attrs(root, node,
+ 											   nappinfos,
+ 											   appinfos));
+ 		}
+ 	}
+ }
+ 
+ /*
+  * build_child_join_reltarget
+  *	  Set up a child-join relation's reltarget from a parent-join relation.
+  */
+ static void
+ build_child_join_reltarget(PlannerInfo *root,
+ 						   RelOptInfo *parentrel,
+ 						   RelOptInfo *childrel,
+ 						   int nappinfos,
+ 						   AppendRelInfo **appinfos)
+ {
+ 	/* Build the targetlist */
+ 	build_childrel_tlist(root, parentrel, childrel, nappinfos, appinfos);
+ 
+ 	/* Set the cost and width fields */
+ 	childrel->reltarget->cost.startup = parentrel->reltarget->cost.startup;
+ 	childrel->reltarget->cost.per_tuple = parentrel->reltarget->cost.per_tuple;
+ 	childrel->reltarget->width = parentrel->reltarget->width;
+ }
*** a/postgres-master/src/include/nodes/relation.h
--- b/postgres-master/src/include/nodes/relation.h
***************
*** 687,694 **** typedef struct RelOptInfo
  								 * involving this rel */
  	bool		has_eclass_joins;	/* T means joininfo is incomplete */
  
! 	/* used by "other" relations */
! 	Relids		top_parent_relids;	/* Relids of topmost parents */
  
--- 687,701 ----
  								 * involving this rel */
  
! 	/* used by partitionwise joins: */
! 	Oid			top_parent_rowtype; /* OID of topmost parent's rowtype */
! 	Relids		top_parent_relids;	/* Relids of topmost parent(s) */
! 	bool		top_parent_parted;	/* T means the topmost parent(s) rel is
! 									 * partitioned */
! 	bool		top_parent_topmost;	/* T means the topmost parent(s) rel is
! 									 * topmost scan/join one */
! 	bool		need_adjust_tlist;	/* T means reltarget->exprs has child
! 									 * whole-row Var(s) */
  
  	/* used for partitioned relations */
  	PartitionScheme part_scheme;	/* Partitioning scheme. */
*** a/postgres-master/src/include/optimizer/pathnode.h
--- b/postgres-master/src/include/optimizer/pathnode.h
***************
*** 297,301 **** extern RelOptInfo *build_child_join_rel(PlannerInfo *root,
--- 297,306 ----
  					 RelOptInfo *outer_rel, RelOptInfo *inner_rel,
  					 RelOptInfo *parent_joinrel, List *restrictlist,
  					 SpecialJoinInfo *sjinfo, JoinType jointype);
+ extern void build_childrel_tlist(PlannerInfo *root,
+ 					 RelOptInfo *parentrel,
+ 					 RelOptInfo *childrel,
+ 					 int nappinfos,
+ 					 AppendRelInfo **appinfos);
  
  #endif							/* PATHNODE_H */
*** a/postgres-master/src/test/regress/expected/partition_join.out
--- b/postgres-master/src/test/regress/expected/partition_join.out
***************
*** 476,481 **** SELECT t1.a, ss.t2a, ss.t2c FROM prt1 t1 LEFT JOIN LATERAL
--- 476,554 ----
   550 |     | 
  (12 rows)
  
+ -- Check whole-row-Var handling in situations where the partitioned rel is not
+ -- the topmost scan/join relation
+ CREATE TABLE nonprt (a int, x text default 'testing');
+ INSERT INTO nonprt (a) SELECT i FROM generate_series(0, 599) i WHERE i % 6 = 0;
+ ANALYZE nonprt;
+ EXPLAIN (COSTS OFF)
+ SELECT t1, t3 FROM prt1 t1, nonprt t3 WHERE t1.a = t3.a AND t1.b = 0 ORDER BY t1.a;
+                     QUERY PLAN                    
+ --------------------------------------------------
+  Sort
+    Sort Key: t1.a
+    ->  Hash Join
+          Hash Cond: (t3.a = t1.a)
+          ->  Seq Scan on nonprt t3
+          ->  Hash
+                ->  Append
+                      ->  Seq Scan on prt1_p1 t1
+                            Filter: (b = 0)
+                      ->  Seq Scan on prt1_p2 t1_1
+                            Filter: (b = 0)
+                      ->  Seq Scan on prt1_p3 t1_2
+                            Filter: (b = 0)
+ (13 rows)
+ 
+ SELECT t1, t3 FROM prt1 t1, nonprt t3 WHERE t1.a = t3.a AND t1.b = 0 ORDER BY t1.a;
+       t1      |      t3       
+ --------------+---------------
+  (0,0,0000)   | (0,testing)
+  (150,0,0150) | (150,testing)
+  (300,0,0300) | (300,testing)
+  (450,0,0450) | (450,testing)
+ (4 rows)
+ 
+ EXPLAIN (COSTS OFF)
+ SELECT t1, t2, t3 FROM prt1 t1, prt2 t2, nonprt t3 WHERE t1.a = t2.b AND t1.a = t3.a AND t1.b = 0 ORDER BY t1.a;
+                           QUERY PLAN                          
+ --------------------------------------------------------------
+  Sort
+    Sort Key: t1.a
+    ->  Hash Join
+          Hash Cond: (t3.a = t1.a)
+          ->  Seq Scan on nonprt t3
+          ->  Hash
+                ->  Append
+                      ->  Hash Join
+                            Hash Cond: (t2.b = t1.a)
+                            ->  Seq Scan on prt2_p1 t2
+                            ->  Hash
+                                  ->  Seq Scan on prt1_p1 t1
+                                        Filter: (b = 0)
+                      ->  Hash Join
+                            Hash Cond: (t2_1.b = t1_1.a)
+                            ->  Seq Scan on prt2_p2 t2_1
+                            ->  Hash
+                                  ->  Seq Scan on prt1_p2 t1_1
+                                        Filter: (b = 0)
+                      ->  Hash Join
+                            Hash Cond: (t2_2.b = t1_2.a)
+                            ->  Seq Scan on prt2_p3 t2_2
+                            ->  Hash
+                                  ->  Seq Scan on prt1_p3 t1_2
+                                        Filter: (b = 0)
+ (25 rows)
+ 
+ SELECT t1, t2, t3 FROM prt1 t1, prt2 t2, nonprt t3 WHERE t1.a = t2.b AND t1.a = t3.a AND t1.b = 0 ORDER BY t1.a;
+       t1      |      t2      |      t3       
+ --------------+--------------+---------------
+  (0,0,0000)   | (0,0,0000)   | (0,testing)
+  (150,0,0150) | (0,150,0150) | (150,testing)
+  (300,0,0300) | (0,300,0300) | (300,testing)
+  (450,0,0450) | (0,450,0450) | (450,testing)
+ (4 rows)
+ 
  --
  -- partitioned by expression
  --
***************
*** 1042,1047 **** SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT *
--- 1115,1163 ----
   400 |    
  (9 rows)
  
+ -- merge join when expression with whole-row reference needs to be sorted
+ EXPLAIN (COSTS OFF)
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+                                     QUERY PLAN                                     
+ -----------------------------------------------------------------------------------
+  Merge Append
+    Sort Key: t1.a
+    ->  Merge Join
+          Merge Cond: ((t1.a = t2.b) AND (((t1.*)::text) = ((t2.*)::text)))
+          ->  Sort
+                Sort Key: t1.a, ((t1.*)::text)
+                ->  Seq Scan on prt1_p1 t1
+          ->  Sort
+                Sort Key: t2.b, ((t2.*)::text)
+                ->  Seq Scan on prt2_p1 t2
+    ->  Merge Join
+          Merge Cond: ((t1_1.a = t2_1.b) AND (((t1_1.*)::text) = ((t2_1.*)::text)))
+          ->  Sort
+                Sort Key: t1_1.a, ((t1_1.*)::text)
+                ->  Seq Scan on prt1_p2 t1_1
+          ->  Sort
+                Sort Key: t2_1.b, ((t2_1.*)::text)
+                ->  Seq Scan on prt2_p2 t2_1
+    ->  Merge Join
+          Merge Cond: ((t1_2.a = t2_2.b) AND (((t1_2.*)::text) = ((t2_2.*)::text)))
+          ->  Sort
+                Sort Key: t1_2.a, ((t1_2.*)::text)
+                ->  Seq Scan on prt1_p3 t1_2
+          ->  Sort
+                Sort Key: t2_2.b, ((t2_2.*)::text)
+                ->  Seq Scan on prt2_p3 t2_2
+ (26 rows)
+ 
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+  a  | b  
+ ----+----
+   0 |  0
+   6 |  6
+  12 | 12
+  18 | 18
+  24 | 24
+ (5 rows)
+ 
  RESET enable_hashjoin;
  RESET enable_nestloop;
  --
*** a/postgres-master/src/test/regress/sql/partition_join.sql
--- b/postgres-master/src/test/regress/sql/partition_join.sql
***************
*** 89,94 **** SELECT t1.a, ss.t2a, ss.t2c FROM prt1 t1 LEFT JOIN LATERAL
--- 89,108 ----
  			  (SELECT t2.a AS t2a, t3.a AS t3a, t2.b t2b, t2.c t2c, least(t1.a,t2.a,t3.a) FROM prt1 t2 JOIN prt2 t3 ON (t2.a = t3.b)) ss
  			  ON t1.c = ss.t2c WHERE (t1.b + coalesce(ss.t2b, 0)) = 0 ORDER BY t1.a;
  
+ -- Check whole-row-Var handling in situations where the partitioned rel is not
+ -- the topmost scan/join relation
+ CREATE TABLE nonprt (a int, x text default 'testing');
+ INSERT INTO nonprt (a) SELECT i FROM generate_series(0, 599) i WHERE i % 6 = 0;
+ ANALYZE nonprt;
+ 
+ EXPLAIN (COSTS OFF)
+ SELECT t1, t3 FROM prt1 t1, nonprt t3 WHERE t1.a = t3.a AND t1.b = 0 ORDER BY t1.a;
+ SELECT t1, t3 FROM prt1 t1, nonprt t3 WHERE t1.a = t3.a AND t1.b = 0 ORDER BY t1.a;
+ 
+ EXPLAIN (COSTS OFF)
+ SELECT t1, t2, t3 FROM prt1 t1, prt2 t2, nonprt t3 WHERE t1.a = t2.b AND t1.a = t3.a AND t1.b = 0 ORDER BY t1.a;
+ SELECT t1, t2, t3 FROM prt1 t1, prt2 t2, nonprt t3 WHERE t1.a = t2.b AND t1.a = t3.a AND t1.b = 0 ORDER BY t1.a;
+ 
  --
  -- partitioned by expression
  --
***************
*** 160,165 **** EXPLAIN (COSTS OFF)
--- 174,184 ----
  SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT * FROM prt2 WHERE b > 250) t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
  SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT * FROM prt2 WHERE b > 250) t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 19 Jul 2018 13:25:03 +0900

(2018/07/13 23:05), Ashutosh Bapat wrote:
>> could be mapped to the former by a technique like do_convert_tuple.

> Can you please show an index only scan path being created in this case?

We currently don't consider index-only scan with index expressions, so I 
haven't thought in detail yet about how the planner would work.  But 
once we have that index-only scan, I think we could extend that to the 
case mentioned above, by adding this to the planner: if the index 
expression is of the form var::parenttype, consider that (not only the 
expression itself but) var can be returned from the index.  I think the 
expression like ptp1::pt::ptp1 would be useful to get the value of ptp1 
from the index at execution time.

> shouldn't require to reverted in a few months from now, just so that

I think we could add that optimization without reverting this change 
because the essential part of this change is to make create_plan adjust 
the tlists of the subplans based on the instruction stored into the 
subplans' RelOptInfos (ie, need_adjust_tlist in the second version of 
the patch).  I think this technique could be extended even to the case 
where we have that optimization.

Best regards,
Etsuro Fujita

---

From: Robert Haas <[email-masked]>
Date: Fri, 20 Jul 2018 08:38:09 -0400

On Wed, Jul 18, 2018 at 8:00 AM, Etsuro Fujita
<[email-masked]> wrote:
> [ new patch ]

+ /*

This looks like a terrible design to me.  If somebody in future
invents a new plan type that is not projection-capable, then this
could fail an assertion here and there won't be any simple fix.  And
if you reach here and the child targetlists somehow haven't been
adjusted, then you won't even fail an assertion, you'll just crash (or
maybe return wrong answers).

I'm going to study this some more now, but I really think this is
going in the wrong direction.

---

From: Robert Haas <[email-masked]>
Date: Fri, 20 Jul 2018 11:26:39 -0400

> maybe return wrong answers).

To elaborate on this thought a bit, it is currently the case that all
scan and join types are projection-capable, and most likely we would
make any such node types added in the future projection-capable as
well, but it still doesn't seem like a good idea to me to have
tangentially-related parts of the system depend on that in subtle
ways.  Also, even today, this would fail if the subplan happened to be
a Sort, and it's not very obvious why that couldn't happen.  If it
can't happen today, it's not obvious why a future code change couldn't
introduce cases where it can happen.

More broadly, the central idea of this patch is that we can set the
target list for a child rel to something other than the target list
that we expect it will eventually produce, and then at plan creation
time fix it.  I think that's a bad idea.  The target list affects lots
of things, like costing.  If we don't insert a ConvertRowTypeExpr into
the child's target list, the costing will be wrong to the extent that
ConvertRowTypeExpr has any cost, which it does.  Any other current or
future property that is computed based on the target list --
parallel-safety, width, security-level, whatever -- could also
potentially end up with a wrong value if the target list as written
does not match the target list as will actually be produced.  It's
true that, in all of these areas, the ConvertRowTypeExpr isn't likely
to have a lot of impact; it probably won't have a big effect on
costing and may not actually cause a problem for the other things that
I mentioned.  On the other hand, if it does cause a serious problem,
what options would we have for fixing it other than to back out your
entire patch and solve the problem some other way?  The idea that
derived properties won't be correct unless they are based on the real
target list is pretty fundamental, and I think deviating from the idea
that we get the correct target list first and then compute the derived
properties afterward is likely to cause real headaches for future
developers.

In short, plan creation time is just the wrong time to change the
plan.  It's just a time to translate the plan from the form needed by
the planner to the form needed by the executor.  It would be fine if
the change you were making were only cosmetic, but it's not.

After looking over both patches, I think Ashutosh Bapat has basically
the right idea.  What he is proposing is a localized fix that doesn't
seem to make any changes to the way things work overall.  I do think
his patches need to be fixed up a bit to avoid conflating
ConvertRowtypeExpr with "converted whole-row reference."  The two are
certainly not equivalent; the latter is a narrow special case.  Some
of the comments could use some more wordsmithing, too, I think.  Apart
from those gripes, though, I think it's solving the problem the
correct way: don't build the wrong plan and try to fix it, just build
the right plan from the beginning.

There are definitely some things not to like about this approach.  In
particular, I definitely agree that treating a converted whole-row
reference specially is not very nice.  It feels like it might be
substantially cleaner to be able to represent this idea as a single
node rather than a combination of ConvertRowtypeExpr and var with
varattno = 0.  Perhaps in the future we ought to consider either (a)
trying to use a RowExpr for this rather than ConvertRowtypeExpr or (b)
inventing a new WholeRowExpr node that stores two RTIs, one for the
relation that's generating the whole-row reference and the other for
the relation that is controlling the column ordering of the result or
(c) allowing a Var to represent a whole-row expression that has to
produce its outputs according to the ordering of some other RTE.  But
I don't think it's wise or necessary to whack that around just to fix
this bug; it is refactoring or improvement work best left to a future
release.

Also, it is definitely a real shame that we have to build attr_needed
data for each child separately.  Right now, we've got to build a whole
lot of things for each child individually, and that's slow and
inefficient.  We're not going to scale nicely to large partitioning
hierarchies unless we can figure out some way of minimizing the work
we do for each partition.  So, the fact that Fujii-san's approach
avoids needing to compute attr_needed in some cases is very appealing.
However, in my opinion, it's nowhere near appealing enough to justify
trying to do surgery on the target list at plan-creation time.  I
think we need to leave optimizing this to a future effort.
Partition-wise join/aggregate, and partitioning in general, need a lot
of optimization in a lot of places, and fortunately there are people
working on that, but our goal here should just be to fix things up
well enough that we can ship it.  I don't see anything in Ashutosh's
patch that is so ugly that we can't live with it for the time being.

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 23 Jul 2018 16:49:03 +0900

(2018/07/21 0:26), Robert Haas wrote:
> On Fri, Jul 20, 2018 at 8:38 AM, Robert Haas<[email-masked]>  wrote:
> ways.

I have to admit that that is not a good idea.  So, I'll update the patch 
so that we don't assume the projection capability of the subplan 
anymore, if we go this way.

> Also, even today, this would fail if the subplan happened to be
> a Sort, and it's not very obvious why that couldn't happen.

You mean the MergeAppend case?  In that case, the patch will adjust the 
subplan before adding a Sort above the subplan, so that could not happen.

> time fix it.

Yeah, the patch would fix the the final output to the appendrel parent 
at plan creation time if necessary, but it would produces the tlist of 
an intermediate child scan/join node as written in the child relation's 
reltarget at that time.

> I think that's a bad idea.  The target list affects lots
> ConvertRowTypeExpr has any cost, which it does.

Actually, this is not true at least currently, because 
set_append_rel_size doesn't do anything about the costing:

          * NB: the resulting childrel->reltarget->exprs may contain 
arbitrary
          * expressions, which otherwise would not occur in a rel's 
targetlist.
          * Code that might be looking at an appendrel child must cope with
          * such.  (Normally, a rel's targetlist would only include Vars and
          * PlaceHolderVars.)  XXX we do not bother to update the cost 
or width
          * fields of childrel->reltarget; not clear if that would be 
useful.

> Any other current or

Some createplan.c routines already change the tlists of their nodes. 
For example, create_merge_append_plan would add sort columns to each of 
its subplans if necessary.  I think it would be similar to that to add a 
ConvertRowtypeExpr above a child whole-row Var in the subplan's tlist.

> seem to make any changes to the way things work overall.

I reviewed his patch, and thought that that would fix the issue, but 
this is about the current design on the child tlist handling in 
partitionise join rather than that patch: it deviates from the 
assumption that we had for the scan/join planning till PG10 that "a 
rel's targetlist would only include Vars and PlaceHolderVars", and turns 
out that there are a lot of places where we need to modify code to 
suppress that deviation, as partly shown in that patch.  So, ISTM that 
the current design in itself is not that localized.

> well enough that we can ship it.

I agree on that point.

Best regards,
Etsuro Fujita

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 23 Jul 2018 15:08:41 +0530

On Fri, Jul 20, 2018 at 8:56 PM, Robert Haas <[email-masked]> wrote:
[ ... clipped portion ...]
> the change you were making were only cosmetic, but it's not.

Agree with all that, including the clipped paras.

> the right plan from the beginning.

I will work on that if everyone agrees that that's the right way to
go. Fujita-san seems to have some arguments still. I have argued on
the same lines as yours but your explanation is better. I don't have
anything more to add. I will wait for that to be resolved.

> produce its outputs according to the ordering of some other RTE.

I never liked representing a whole-row expression as a Var worst with
varattno = 0. We should have always casted it as RowExpr(set of vars,
one var per column). This problem wouldn't arise then. Many other
problems wouldn't arise then, I think. I think we do that so that we
can convert a tuple from buffer into a datum and put it in place of
Var with varattno = 0. Given that I prefer (a) or (b) in all the
cases. If not that, then c. But I agree that we have to avoid
ConvertRowtypeExpr being used in this case.

> But

I agree.

> I don't see anything in Ashutosh's

Thanks.

---

From: Robert Haas <[email-masked]>
Date: Mon, 23 Jul 2018 14:09:43 -0400

On Mon, Jul 23, 2018 at 3:49 AM, Etsuro Fujita
<[email-masked]> wrote:
> I have to admit that that is not a good idea.  So, I'll update the patch so
> that we don't assume the projection capability of the subplan anymore, if we
> go this way.

Isn't that assumption fundamental to your whole approach?

> subplan before adding a Sort above the subplan, so that could not happen.

That would only help if create_merge_append_plan() itself inserted a
Sort node.  It wouldn't help if the Sort node came from a child path
manufactured by create_sort_path().

> Actually, this is not true at least currently, because set_append_rel_size
> doesn't do anything about the costing:

Why would it?  Append can't project, so the cost of any expressions
that appear in its target list is irrelevant.  What is affected is the
cost of the scans below the Append -- see e.g. cost_seqscan(), which
uses the data produced by set_pathtarget_cost_width().

> Some createplan.c routines already change the tlists of their nodes. For
> example, create_merge_append_plan would add sort columns to each of its
> subplans if necessary.  I think it would be similar to that to add a

You have a point, but I think that code is actually not a very good
idea, and I'd like to see us do less of that sort of thing, not more.
Any case in which we change the plan while creating it has many of the
same problems that I discussed in the previous email.  For example,
create_merge_append_path() has to know that a Sort node might get
inserted and set the costing accordingly.  If the callers guaranteed
that the necessary sort path had already been inserted, then we
wouldn't need that special handling.

Also, that code is adding additional columns, computed from the
columns we have available, so that we can sort.  Those extra columns
then get discarded at the next level of the Plan tree.  What you're
trying to do is different.  Perhaps this is too harsh a judgement, but
it looks to me like you're using a deliberately-wrong representation
of the value that you ultimately want to produce and then patching it
up after the fact.  That seems quite a bit worse than what the
existing code is doing.

> I reviewed his patch, and thought that that would fix the issue, but this is
> about the current design on the child tlist handling in partitionise join
> rather than that patch: it deviates from the assumption that we had for the
> scan/join planning till PG10 that "a rel's targetlist would only include
> Vars and PlaceHolderVars", and turns out that there are a lot of places
> where we need to modify code to suppress that deviation, as partly shown in
> that patch.  So, ISTM that the current design in itself is not that
> localized.

I used to think that was the assumption, too, but it seems to me that
Tom told me a while back that it was never really true, and that
assumption was in my head.  Unfortunately, I don't have a link to that
email handy.  Either way, I think the solution is to get the tlist
right from the start and cope with the consequences downstream, not
start with the wrong thing and try to fix it later.  To see an example
of why I think that's a valuable approach, see
11cf92f6e2e13c0a6e3f98be3e629e6bd90b74d5, especially the changes in
the regression test outputs.  The old code discovered after it had
generated an Append that it had the wrong tlist, but since the Append
can't project, it had to add a Result node.  With the new code, we get
the children of the Append to produce the right output from the start,
and then the Append just needs to concatenate all that output, so no
Result node is needed.  As noted in the commit message, it also made
the costing more accurate.

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 24 Jul 2018 20:51:36 +0900

> Isn't that assumption fundamental to your whole approach?

I don't think so.  What I mean here is: currently the subplan would be a 
scan/join node, but in future we might have eg, a Sort node atop the 
scan/join node, so it would be better to update the patch to handle such 
a case as well.

> manufactured by create_sort_path().

As I mentioned above, I think we could handle such a case as well.

> uses the data produced by set_pathtarget_cost_width().

By set_rel_size()?

> wouldn't need that special handling.

I'm not sure that's a good idea, because I think we have a trade-off 
relation; the more we make create_plan simple, the more we need to make 
earlier states of the planner complicated.

And it looks to me like the partitionwise join code is making earlier 
(and later) stages of the planner too complicated, to make create_plan 
simple.

> up after the fact.

When considering paritionwise joining, it would make things complicated 
to have a ConvertRowtypeExpr in a partrel's targetlist, because as 
discussed upthread, it deviates from the planner's assumption that a 
rel's targetlist would only include Vars and PHVs.  So, I propose to 
include a child whole-row Var in the targetlist instead, in which case, 
we need to fix the Var after the fact, but can avoid making many other 
parts of the planner complicated.

> That seems quite a bit worse than what the
> existing code is doing.

One idea to address that concern would be to adjust the pathtarget of 
each path created in generate_partitionwise_join_paths accordingly.  I'm 
missing something, though.

> start with the wrong thing and try to fix it later.

I'm not fully convinced that's really the right solution, but I think 
you know better about partitioning (as well as many other things) than 
me, so I think it's better for you to decide what to do for this issue.

Thanks for working on this!

Best regards,
Etsuro Fujita

---

From: Robert Haas <[email-masked]>
Date: Wed, 25 Jul 2018 16:27:41 -0400

On Tue, Jul 24, 2018 at 7:51 AM, Etsuro Fujita
<[email-masked]> wrote:
> scan/join node, so it would be better to update the patch to handle such a
> case as well.

But how would you do that?

>> uses the data produced by set_pathtarget_cost_width().

Sorry, I don't understand what you mean by this.

> And it looks to me like the partitionwise join code is making earlier (and
> later) stages of the planner too complicated, to make create_plan simple.

I think that create_plan is *supposed* to be simple.  Its purpose is
to prune away data that's only needed during planning and add things
that can be computed at the last minute which are needed at execution
time.  Making it do anything else is, in my opinion, not good.

> When considering paritionwise joining, it would make things complicated to
> have a ConvertRowtypeExpr in a partrel's targetlist, because as discussed
> upthread, it deviates from the planner's assumption that a rel's targetlist
> would only include Vars and PHVs.  So, I propose to include a child
> whole-row Var in the targetlist instead, in which case, we need to fix the
> Var after the fact, but can avoid making many other parts of the planner
> complicated.

Well, I could have the wrong idea here, but I tend to think allowing
for ConvertRowTypeExpr elsewhere won't be that bad.

Does anyone else want to weigh in on this issue?  Tom, perhaps?

---

From: Tom Lane <[email-masked]>
Date: Wed, 25 Jul 2018 16:42:41 -0400

> time.  Making it do anything else is, in my opinion, not good.

I tend to agree with Robert on this.  In particular, if you put anything
into create_plan or later that affects cost estimates, you're really doing
it wrong, because changing those estimates might've changed the plan
entirely.  (As I recall, we have a couple of cheats like that now,
associated with dropping no-op projection and subquery scan nodes.
But let's not add more.)

TBH, I think this entire discussion is proving what sheer folly it was
to allow partitions to have rowtypes not identical to their parent.
But I suppose we're stuck with that now.

			regards, tom lane

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 26 Jul 2018 11:01:51 +0530

+1

> But I suppose we're stuck with that now.

Every table created has a different rowtype and when the partition's
rowtype is different from that of the partitioned table, we add a
ConvertRowtypeExpr node on top of the whole-row expression.

2153                     if (appinfo->parent_reltype != appinfo->child_reltype)
2154                     {
2155                         ConvertRowtypeExpr *r =
makeNode(ConvertRowtypeExpr);

We may be able to set rowtype of a partition to be same as the rowtype
of a partitioned table when a partition table is created using an
ALTER TABLE syntax. But are you suggesting that we change the reltype
of a partition being attached? What happens when we detach a partition
and let the table live independent of the partitioned table? Do we
create a new reltype?

I am not even going into the problem when we try to attach a partition
with different tuple layout. I do believe that such usecases arise and
rewriting the table being attached is a viable option.

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 26 Jul 2018 21:11:01 +0900

> But how would you do that?

What I had in mind was to insert a Rusult node with 
inject_projection_plan and adjust the tlist of the Result, as done for 
adding sort columns to a tlist in prepare_sort_from_pathkeys.

> Sorry, I don't understand what you mean by this.

I think the data used by such a costing function is computed by 
set_rel_size in set_append_rel_size, not set_pathtarget_cost_width; in 
the case of a plain partition, for example, set_rel_size would call 
set_plain_rel_size, and then set_plain_rel_size would eventually call 
set_rel_width, which sets reltarget->cost, which I think would be used 
by e.g., cost_seqscan.  cost_qual_eval_node, which is called in 
set_rel_width for computing the cost of ConvertRowTypeExpr, ignores that 
expression, so currently, we don't charge any cost for it to the 
partition's reltarget->cost, and to the cost of a scan below the Append.

I agree on that point.

> for ConvertRowTypeExpr elsewhere won't be that bad.

I still don't like that because in my opinion, changes needed for that 
would not be localized, and that would make code complicated more than 
necessary.

As I mentioned in a previous email, another idea to avoid that would be 
to adjust tlists for children at path creation time, not plan creation 
time; we could adjust the tlist for each of subpaths accumulated for an 
Append/MergeAppend path in add_paths_to_append_rel called from 
set_append_rel_pathlist or generate_partitionwise_join_paths, with 
create_projection_path adding ConvertRowTypeExpr.  It seems unlikely 
that performing create_projection_path to such a subpath would change 
its property of being the cheapest, so it would be safe to adjust the 
tlists that way.  This would not require making create_plan complicated 
anymore.  I might be missing something, though.

Best regards,
Etsuro Fujita

---

From: Andres Freund <[email-masked]>
Date: Mon, 30 Jul 2018 12:06:50 -0700

Hi,

> going in the wrong direction.

We're going to have to get somewhere on this topic soon. This thread has
been open for nearly half a year, and we're late in the beta phase now.
What can we do to get to an agreement soon?

Greetings,

Andres Freund

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 31 Jul 2018 10:43:50 +0900

> What can we do to get to an agreement soon?

I'd like to propose an updated patch as proposed in [1].

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5B59BA55.30200%40lab.ntt.co.jp

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 31 Jul 2018 16:31:24 -0400

> I'd like to propose an updated patch as proposed in [1].

But there is no patch there, and neither there is agreement from the
other party that the approach described there is sound.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 01 Aug 2018 20:44:35 +0900

(2018/07/26 21:11), Etsuro Fujita wrote:
> create_projection_path adding ConvertRowTypeExpr. It seems unlikely that
> performing create_projection_path to such a subpath would change its
> property of being the cheapest, so it would be safe to adjust the tlists
> that way. This would not require making create_plan complicated anymore.
> I might be missing something, though.

I updated the patch that way.  Updated patch attached.  I fixed a bug 
and did a bit of cleanups as well.

Best regards,
Etsuro Fujita

  

  
  
+ -- test FOR UPDATE
+ EXPLAIN (COSTS OFF)

  

+ -- test FOR UPDATE
+ EXPLAIN (COSTS OFF)

*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 2359,2365 **** _outRelOptInfo(StringInfo str, const RelOptInfo *node)
--- 2359,2368 ----
  	WRITE_UINT_FIELD(baserestrict_min_security);
  	WRITE_NODE_FIELD(joininfo);
  	WRITE_BOOL_FIELD(has_eclass_joins);
+ 	WRITE_BOOL_FIELD(consider_partitionwise_join);
+ 	WRITE_OID_FIELD(top_parent_rowtype);
  	WRITE_BITMAPSET_FIELD(top_parent_relids);
+ 	WRITE_BOOL_FIELD(has_target_wrvs);
  	WRITE_NODE_FIELD(partitioned_child_rels);
  }
  
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***************
*** 910,915 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
--- 910,928 ----
  	}
  
  	/*
+ 	 * If this is a baserel, set the consider_partitionwise_join flag and
+ 	 * check whether the baserel's targetlist contain a whole-row Var.
+ 	 */
+ 	if (rel->reloptkind == RELOPT_BASEREL)
+ 	{
+ 		if (enable_partitionwise_join && rel->part_scheme &&
+ 			!bms_equal(rel->relids, root->all_baserels))
+ 			rel->consider_partitionwise_join = true;
+ 		if (rel->attr_needed[InvalidAttrNumber - rel->min_attr] != NULL)
+ 			rel->has_target_wrvs = true;
+ 	}
+ 
+ 	/*
  	 * Initialize to compute size estimates for whole append relation.
  	 *
  	 * We handle width estimates by weighting the widths of different child
***************
*** 956,1025 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  		childrel = find_base_rel(root, childRTindex);
  		Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL);
  
- 		if (rel->part_scheme)
- 		{
- 			AttrNumber	attno;
- 
- 			/*
  
  		/*
--- 969,983 ----
  
  		/*
  
  		/*
--- 1139,1152 ----
  								   1, &appinfo);
  
  		/*
  
--- 41,46 ----
***************

  
  
  	if (node == NULL)
  		return NULL;
--- 2245,2250 ----
***************
*** 2326,2337 **** fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
  	}
  	if (IsA(node, Param))
  		return fix_param_node(context->root, (Param *) node);
- 
  	/* Try matching more complex expressions too, if tlists have any */
! 	converted_whole_row = is_converted_whole_row_reference(node);
! 	if (context->outer_itlist &&
! 		(context->outer_itlist->has_non_vars ||
! 		 (context->outer_itlist->has_conv_whole_rows && converted_whole_row)))
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->outer_itlist,
--- 2314,2321 ----
  	}
  	if (IsA(node, Param))
  		return fix_param_node(context->root, (Param *) node);
  	/* Try matching more complex expressions too, if tlists have any */
! 	if (context->outer_itlist && context->outer_itlist->has_non_vars)
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->outer_itlist,
***************
*** 2339,2347 **** fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
  		if (newvar)
  			return (Node *) newvar;
  	}
! 	if (context->inner_itlist &&
! 		(context->inner_itlist->has_non_vars ||
! 		 (context->inner_itlist->has_conv_whole_rows && converted_whole_row)))
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->inner_itlist,
--- 2323,2329 ----
  		if (newvar)
  			return (Node *) newvar;
  	}
! 	if (context->inner_itlist && context->inner_itlist->has_non_vars)
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->inner_itlist,
***************
*** 2461,2469 **** fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
  		/* If no match, just fall through to process it normally */
  	}
  	/* Try matching more complex expressions too, if tlist has any */
! 	if (context->subplan_itlist->has_non_vars ||
! 		(context->subplan_itlist->has_conv_whole_rows &&
! 		 is_converted_whole_row_reference(node)))
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->subplan_itlist,
--- 2443,2449 ----
  		/* If no match, just fall through to process it normally */
  	}
  	/* Try matching more complex expressions too, if tlist has any */
! 	if (context->subplan_itlist->has_non_vars)
  	{
  		newvar = search_indexed_tlist_for_non_var((Expr *) node,
  												  context->subplan_itlist,
***************
*** 2670,2702 **** extract_query_dependencies_walker(Node *node, PlannerInfo *context)
  	return expression_tree_walker(node, extract_query_dependencies_walker,
  								  (void *) context);
  }
- 
- /*
-  * is_converted_whole_row_reference
-  *		If the given node is a ConvertRowtypeExpr encapsulating a whole-row
-  *		reference as implicit cast, return true. Otherwise return false.
-  */
- static bool
- is_converted_whole_row_reference(Node *node)
- {
- 	ConvertRowtypeExpr *convexpr;
- 
- 	if (!node || !IsA(node, ConvertRowtypeExpr))
- 		return false;
- 
- 	/* Traverse nested ConvertRowtypeExpr's. */
- 	convexpr = castNode(ConvertRowtypeExpr, node);
- 	while (convexpr->convertformat == COERCE_IMPLICIT_CAST &&
- 		   IsA(convexpr->arg, ConvertRowtypeExpr))
- 		convexpr = castNode(ConvertRowtypeExpr, convexpr->arg);
- 
- 	if (IsA(convexpr->arg, Var))
- 	{
- 		Var		   *var = castNode(Var, convexpr->arg);
- 
- 		if (var->varattno == 0)
- 			return true;
- 	}
- 
- 	return false;
- }
--- 2650,2652 ----
*** a/src/backend/optimizer/util/pathnode.c
--- b/src/backend/optimizer/util/pathnode.c
***************
*** 53,58 **** typedef enum
--- 53,62 ----
  static List *translate_sub_tlist(List *tlist, int relid);
  static int	append_total_cost_compare(const void *a, const void *b);
  static int	append_startup_cost_compare(const void *a, const void *b);
+ static List *add_projection_to_subpaths(PlannerInfo *root, RelOptInfo *rel,
+ 						   List *subpaths);
+ static Path *add_projection_to_subpath(PlannerInfo *root, RelOptInfo *rel,
+ 						  Path *subpath);
  static List *reparameterize_pathlist_by_child(PlannerInfo *root,
  								 List *pathlist,
  								 RelOptInfo *child_rel);
***************
*** 1274,1279 **** create_append_path(PlannerInfo *root,
--- 1278,1288 ----
  	pathnode->first_partial_path = list_length(subpaths);
  	pathnode->subpaths = list_concat(subpaths, partial_subpaths);
  
+ 	/* Add a projection step to each subpath if needed. */
+ 	if (rel->consider_partitionwise_join && rel->has_target_wrvs)
+ 		pathnode->subpaths = add_projection_to_subpaths(root, rel,
+ 														pathnode->subpaths);
+ 
  	foreach(l, pathnode->subpaths)
  	{
  		Path	   *subpath = (Path *) lfirst(l);
***************
*** 1366,1371 **** create_merge_append_path(PlannerInfo *root,
--- 1375,1384 ----
  	pathnode->path.parallel_workers = 0;
  	pathnode->path.pathkeys = pathkeys;
  	pathnode->partitioned_rels = list_copy(partitioned_rels);
+ 
+ 	/* Add a projection step to each subpath if needed. */
+ 	if (rel->consider_partitionwise_join && rel->has_target_wrvs)
+ 		subpaths = add_projection_to_subpaths(root, rel, subpaths);
  	pathnode->subpaths = subpaths;
  
  	/*
***************
*** 1429,1434 **** create_merge_append_path(PlannerInfo *root,
--- 1442,1524 ----
  }
  
  /*
+  * add_projection_to_subpaths
+  *	  Add a projection step for rowtype conversion to given subpaths.
+  *
+  * This function adjusts subpaths accumulated for creating a partitionwise
+  * join path, by adding a projection to each subpath to convert the tuple
+  * layouts of whole-row Vars in the targetlist for that subpath back to their
+  * respective parents' rowtypes.  Note that that would be safe, because it
+  * seems unlikely that adding that projection to such a subpath would alter
+  * the parallel safety, the ordering (pathkeys), the parameterization
+  * (required_outer rels), and the property of being the cheapest.
+  */
+ static List *
+ add_projection_to_subpaths(PlannerInfo *root, RelOptInfo *rel, List *subpaths)
+ {
+ 	List	   *result = NIL;
+ 	ListCell   *l;
+ 
+ 	Assert(rel->consider_partitionwise_join && rel->has_target_wrvs);
+ 
+ 	foreach(l, subpaths)
+ 	{
+ 		Path	   *subpath = (Path *) lfirst(l);
+ 
+ 		result = lappend(result,
+ 						 add_projection_to_subpath(root, rel, subpath));
+ 	}
+ 	return result;
+ }
+ 
+ /*
+  * add_projection_to_subpath
+  *	  Add a projection step for rowtype conversion to given subpath.
+  */
+ static Path *
+ add_projection_to_subpath(PlannerInfo *root, RelOptInfo *rel, Path *subpath)
+ {
+ 	PathTarget *target = copy_pathtarget(subpath->pathtarget);
+ 	List	   *new_exprs = NIL;
+ 	ListCell   *parentvars;
+ 	ListCell   *childvars;
+ 
+ 	Assert(subpath->parent->has_target_wrvs);
+ 
+ 	/*
+ 	 * Build a new targetlist for the subpath by adding a conversion node
+ 	 * above each (converted) whole-row Var in the subpath's targetlist to
+ 	 * convert the tuple layout of that Var back to its parent rowtype.
+ 	 *
+ 	 * By construction, subpath's targetlist is 1-to-1 with parent's.
+ 	 */
+ 	forboth(parentvars, rel->reltarget->exprs, childvars, target->exprs)
+ 	{
+ 		Var		   *parentvar = (Var *) lfirst(parentvars);
+ 		Node	   *childvar = (Node *) lfirst(childvars);
+ 
+ 		if (IsA(parentvar, Var) && parentvar->varattno == 0)
+ 		{
+ 			ConvertRowtypeExpr *cre = makeNode(ConvertRowtypeExpr);
+ 
+ 			cre->arg = (Expr *) childvar;
+ 			cre->resulttype = parentvar->vartype;
+ 			cre->convertformat = COERCE_IMPLICIT_CAST;
+ 			cre->location = -1;
+ 			new_exprs = lappend(new_exprs, cre);
+ 		}
+ 		else
+ 			new_exprs = lappend(new_exprs, childvar);
+ 	}
+ 	target->exprs = new_exprs;
+ 
+ 	return (Path *) create_projection_path(root,
+ 										   subpath->parent,
+ 										   subpath,
+ 										   target);
+ }
+ 
+ /*
   * create_result_path
   *	  Creates a path representing a Result-and-nothing-else plan.
   *
***************
*** 2392,2399 **** create_projection_path(PlannerInfo *root,
  	pathnode->path.pathtype = T_Result;
  	pathnode->path.parent = rel;
  	pathnode->path.pathtarget = target;
! 	/* For now, assume we are above any joins, so no parameterization */
! 	pathnode->path.param_info = NULL;
  	pathnode->path.parallel_aware = false;
  	pathnode->path.parallel_safe = rel->consider_parallel &&
  		subpath->parallel_safe &&
--- 2482,2488 ----
  	pathnode->path.pathtype = T_Result;
  	pathnode->path.parent = rel;
  	pathnode->path.pathtarget = target;
! 	pathnode->path.param_info = subpath->param_info;
  	pathnode->path.parallel_aware = false;
  	pathnode->path.parallel_safe = rel->consider_parallel &&
  		subpath->parallel_safe &&
*** a/src/backend/optimizer/util/placeholder.c
--- b/src/backend/optimizer/util/placeholder.c
***************
*** 20,26 ****
  #include "optimizer/pathnode.h"
  #include "optimizer/placeholder.h"
  #include "optimizer/planmain.h"
- #include "optimizer/prep.h"
  #include "optimizer/var.h"
  #include "utils/lsyscache.h"
  
--- 20,25 ----
***************

  #include <limits.h>

  typedef struct JoinHashEntry
***************
*** 37,43 **** typedef struct JoinHashEntry
  } JoinHashEntry;
  
  static void build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
! 					RelOptInfo *input_rel);
  static List *build_joinrel_restrictlist(PlannerInfo *root,
  						   RelOptInfo *joinrel,
  						   RelOptInfo *outer_rel,
--- 39,45 ----
  } JoinHashEntry;
  
  static void build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
! 					RelOptInfo *input_rel, bool *has_wholerow);
  static List *build_joinrel_restrictlist(PlannerInfo *root,
  						   RelOptInfo *joinrel,
  						   RelOptInfo *outer_rel,
***************
*** 57,62 **** static void add_join_rel(PlannerInfo *root, RelOptInfo *joinrel);
--- 59,69 ----
  static void build_joinrel_partition_info(RelOptInfo *joinrel,
  							 RelOptInfo *outer_rel, RelOptInfo *inner_rel,
  							 List *restrictlist, JoinType jointype);
+ static void build_child_join_reltarget(PlannerInfo *root,
+ 						   RelOptInfo *parentrel,
+ 						   RelOptInfo *childrel,
+ 						   int nappinfos,
+ 						   AppendRelInfo **appinfos);

  /*
***************
--- 195,204 ----
+ 	rel->consider_partitionwise_join = false; /* might get changed later */
+ 	rel->has_target_wrvs = false;
--- 212,219 ----
! 	 * also an indirect child of that parent.  Likeweise for the OID of that
--- 221,237 ----
  
+ 		if (rte->relkind == RELKIND_PARTITIONED_TABLE)
+ 			rel->top_parent_rowtype = get_rel_type_id(rte->relid);
+ 	}
+ 
  	/* Check type of rtable entry */
  	switch (rte->rtekind)
  	{
***************
*** 525,530 **** build_join_rel(PlannerInfo *root,
--- 544,550 ----
  {
  	RelOptInfo *joinrel;
  	List	   *restrictlist;
+ 	bool		has_wholerow;
  
  	/* This function should be used only for join between parents. */
  	Assert(!IS_OTHER_REL(outer_rel) && !IS_OTHER_REL(inner_rel));
***************
*** 602,608 **** build_join_rel(PlannerInfo *root,
--- 622,631 ----
  	joinrel->baserestrict_min_security = UINT_MAX;
  	joinrel->joininfo = NIL;
  	joinrel->has_eclass_joins = false;
+ 	joinrel->consider_partitionwise_join = false; /* might get changed later */
+ 	joinrel->top_parent_rowtype = InvalidOid;
  	joinrel->top_parent_relids = NULL;
+ 	joinrel->has_target_wrvs = false;
  	joinrel->part_scheme = NULL;
  	joinrel->nparts = 0;
  	joinrel->boundinfo = NULL;
***************
*** 623,630 **** build_join_rel(PlannerInfo *root,
  	 * and inner rels we first try to build it from.  But the contents should
  	 * be the same regardless.
  	 */
! 	build_joinrel_tlist(root, joinrel, outer_rel);
! 	build_joinrel_tlist(root, joinrel, inner_rel);
  	add_placeholders_to_joinrel(root, joinrel, outer_rel, inner_rel);
  
  	/*
--- 646,654 ----
  	 * and inner rels we first try to build it from.  But the contents should
  	 * be the same regardless.
  	 */
! 	has_wholerow = false;
! 	build_joinrel_tlist(root, joinrel, outer_rel, &has_wholerow);
! 	build_joinrel_tlist(root, joinrel, inner_rel, &has_wholerow);
  	add_placeholders_to_joinrel(root, joinrel, outer_rel, inner_rel);
  
  	/*
***************
*** 660,665 **** build_join_rel(PlannerInfo *root,
--- 684,698 ----
  	build_joinrel_partition_info(joinrel, outer_rel, inner_rel, restrictlist,
  								 sjinfo->jointype);
  
+ 	/* Set the consider_partitionwise_join flag. */
+ 	if (IS_PARTITIONED_REL(joinrel) &&
+ 		!bms_equal(joinrel->relids, root->all_baserels))
+ 		joinrel->consider_partitionwise_join = true;
+ 
+ 	/* Check whether the joinrel's targetlist contain whole-row Vars. */
+ 	if (has_wholerow)
+ 		joinrel->has_target_wrvs = true;
+ 
  	/*
  	 * Set estimates of the joinrel's size.
  	 */
***************
*** 773,779 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
--- 806,815 ----
  	joinrel->baserestrictcost.per_tuple = 0;
  	joinrel->joininfo = NIL;
  	joinrel->has_eclass_joins = false;
+ 	joinrel->consider_partitionwise_join = false; /* might get changed later */
+ 	joinrel->top_parent_rowtype = InvalidOid;
  	joinrel->top_parent_relids = NULL;
+ 	joinrel->has_target_wrvs = false;
  	joinrel->part_scheme = NULL;
  	joinrel->nparts = 0;
  	joinrel->boundinfo = NULL;
***************
*** 789,802 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
  	/* Compute information relevant to foreign relations. */
  	set_foreign_rel_properties(joinrel, outer_rel, inner_rel);
  
! 	/* Build targetlist */
  
--- 825,837 ----

  	/* Construct joininfo list. */
  	joinrel->joininfo = (List *) adjust_appendrel_attrs(root,
  														(Node *) parent_joinrel->joininfo,
  														nappinfos,
***************
*** 889,909 **** min_join_parameterization(PlannerInfo *root,
   * Vars from the specified input rel's tlist to the join rel's tlist.
   *
   * We also compute the expected width of the join's output, making use
!  * of data that was cached at the baserel level by set_rel_width().
   */
  static void
  build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
! 					RelOptInfo *input_rel)
  {
! 	Relids		relids;
  	ListCell   *vars;
  
- 	/* attrs_needed refers to parent relids and not those of a child. */
- 	if (joinrel->top_parent_relids)
- 		relids = joinrel->top_parent_relids;
- 	else
- 		relids = joinrel->relids;
- 
  	foreach(vars, input_rel->reltarget->exprs)
  	{
  		Var		   *var = (Var *) lfirst(vars);
--- 924,940 ----
   * Vars from the specified input rel's tlist to the join rel's tlist.
   *
   * We also compute the expected width of the join's output, making use
!  * of data that was cached at the baserel level by set_rel_width(), and
!  * check whether the join's output contains whole-row Vars for later use
!  * in partitionwise joins.
   */
  static void
  build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
! 					RelOptInfo *input_rel, bool *has_wholerow)
  {
! 	Relids		relids = joinrel->relids;
  	ListCell   *vars;
  
  	foreach(vars, input_rel->reltarget->exprs)
  	{
  		Var		   *var = (Var *) lfirst(vars);
***************
*** 919,973 **** build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
  
  		/*

--- 950,977 ----
  
  		/*

+ 			/* If it's a whole-row Var, set the flag */
+ 			if (var->varattno == InvalidAttrNumber)
+ 				*has_wholerow = true;
--- 1219,1227 ----
  
+ 	upperrel->consider_partitionwise_join = false;
+ 	upperrel->has_target_wrvs = false;
+ 
  	root->upper_rels[kind] = lappend(root->upper_rels[kind], upperrel);
  
  	return upperrel;
***************
*** 1768,1770 **** build_joinrel_partition_info(RelOptInfo *joinrel, RelOptInfo *outer_rel,
--- 1775,1893 ----
  		joinrel->nullable_partexprs[cnt] = nullable_partexpr;
  	}
  }
+ 
+ /*
+  * build_childrel_tlist
+  *	  Build the child's targetlist from the parent's with
+  *	  adjust_appendrel_attrs
+  */
+ void
+ build_childrel_tlist(PlannerInfo *root,
+ 					 RelOptInfo *parentrel,
+ 					 RelOptInfo *childrel,
+ 					 int nappinfos,
+ 					 AppendRelInfo **appinfos)
+ {
+ 	ListCell   *lc;
+ 
+ 	/*
+ 	 * If we don't consider partitionwise join paths, or if the parent's
+ 	 * targetlist doesn't contain any whole-row Vars, just apply
+ 	 * adjust_appendrel_attrs to the parent's targetlist.
+ 	 *
+ 	 * NB: the resulting childrel->reltarget->exprs may contain arbitrary
+ 	 * expressions, which otherwise would not occur in a rel's targetlist.
+ 	 * Code that might be looking at an appendrel child must cope with such.
+ 	 * (Normally, a rel's targetlist would only include Vars and
+ 	 * PlaceHolderVars.)  XXX we do not bother to update the cost or width
+ 	 * fields of childrel->reltarget; not clear if that would be useful.
+ 	 */
+ 	if (!parentrel->consider_partitionwise_join ||
+ 		!parentrel->has_target_wrvs)
+ 	{
+ 		childrel->reltarget->exprs = (List *)
+ 			adjust_appendrel_attrs(root,
+ 								   (Node *) parentrel->reltarget->exprs,
+ 								   nappinfos, appinfos);
+ 		return;
+ 	}
+ 
+ 	/* The parent should be partitioned */
+ 	Assert(parentrel->part_scheme);
+ 
+ 	/*
+ 	 * We build the child's targetlist not to contain ConvertRowtypeExprs:
+ 	 * if the entry in the parent's targetlist is a whole-row Var for one of
+ 	 * the parent's rels, create a whole-row Var for the corresponding child
+ 	 * rel, instead of applying adjust_appendrel_attrs to the Var, and add it
+ 	 * to the child's targetlist; otherwise apply adjust_appendrel_attrs to
+ 	 * the entry in that targetlist.
+ 	 *
+ 	 * We also create an output that should be emitted by the child's rel for
+ 	 * the topmost parent's rel.
+ 	 */
+ 	foreach(lc, parentrel->reltarget->exprs)
+ 	{
+ 		Node	   *node = (Node *) lfirst(lc);
+ 
+ 		/* Should be a simple Var or PlaceHolderVar ... */
+ 		Assert(IsA(node, Var) || IsA(node, PlaceHolderVar));
+ 
+ 		if (IsA(node, Var) && ((Var *) node)->varattno == 0)
+ 		{
+ 			Var		   *var = (Var *) copyObject(node);
+ 			AppendRelInfo *appinfo = NULL;
+ 			int			cnt;
+ 
+ 			/* Find the AppendRelInfo associated with the parent */
+ 			for (cnt = 0; cnt < nappinfos; cnt++)
+ 			{
+ 				if (var->varno == appinfos[cnt]->parent_relid)
+ 				{
+ 					appinfo = appinfos[cnt];
+ 					break;
+ 				}
+ 			}
+ 			/* Should have found the entry ... */
+ 			Assert(appinfo);
+ 			var->varno = appinfo->child_relid;
+ 			var->varnoold = appinfo->child_relid;
+ 			var->vartype = appinfo->child_reltype;
+ 			childrel->reltarget->exprs =
+ 				lappend(childrel->reltarget->exprs, var);
+ 			childrel->has_target_wrvs = true;
+ 		}
+ 		else
+ 		{
+ 			childrel->reltarget->exprs =
+ 				lappend(childrel->reltarget->exprs,
+ 						adjust_appendrel_attrs(root, node,
+ 											   nappinfos,
+ 											   appinfos));
+ 		}
+ 	}
+ 	Assert(childrel->has_target_wrvs);
+ 
+ 	if (childrel->part_scheme)
+ 		childrel->consider_partitionwise_join = true;
+ }
+ 
+ /*
+  * build_child_join_reltarget
+  *	  Set up a child-join relation's reltarget from a parent-join relation.
+  */
+ static void
+ build_child_join_reltarget(PlannerInfo *root,
+ 						   RelOptInfo *parentrel,
+ 						   RelOptInfo *childrel,
+ 						   int nappinfos,
+ 						   AppendRelInfo **appinfos)
+ {
+ 	/* Build the targetlist */
+ 	build_childrel_tlist(root, parentrel, childrel, nappinfos, appinfos);
+ 
+ 	/* Set the cost and width fields */
+ 	childrel->reltarget->cost.startup = parentrel->reltarget->cost.startup;
+ 	childrel->reltarget->cost.per_tuple = parentrel->reltarget->cost.per_tuple;
+ 	childrel->reltarget->width = parentrel->reltarget->width;
+ }
*** a/src/include/nodes/relation.h
--- b/src/include/nodes/relation.h
***************
*** 687,694 **** typedef struct RelOptInfo
  								 * involving this rel */
  	bool		has_eclass_joins;	/* T means joininfo is incomplete */

  
! 	/* per-partitioned-relation planner control flags */
! 	bool		consider_partitionwise_join;	/* consider partitionwise
! 												 * join paths? */
! 
! 	/* used by partitionwise joins: */
! 	Oid			top_parent_rowtype; /* OID of topmost parent's rowtype */
! 	Relids		top_parent_relids;	/* Relids of topmost parent(s) */
! 	bool		has_target_wrvs;	/* T means reltarget->exprs contains
! 									 * whole-row Vars */

  #endif							/* PATHNODE_H */

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 01 Aug 2018 20:46:48 +0900

> other party that the approach described there is sound.

I posted the updated patch [1].

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5B619D23.4010000%40lab.ntt.co.jp

---

From: Robert Haas <[email-masked]>
Date: Wed, 1 Aug 2018 13:44:07 -0400

> I posted the updated patch [1].

Etsuro-san:

I really think we should just go with what Ashutosh had proposed.
Tom, Ashutosh, and I all seem to agree that we shouldn't try to
re-jigger things at create-plan time.  I think that a 3-1 consensus
against your proposal is sufficient to say we shouldn't go that way.

Now, here you've got a new approach which avoids that, which I have
not yet reviewed.  I'll try to spend some time on it this afternoon,
but really, I think it's too late for this.  This bug was reported in
February, and we're supposed to be pushing 11 final out the door in
not much more than a month.  Proposing a new approach in August is not
good.  Can't we just do what Ashutosh proposed for now and revisit
this for v12?

---

From: Robert Haas <[email-masked]>
Date: Wed, 1 Aug 2018 15:30:07 -0400

On Wed, Aug 1, 2018 at 7:44 AM, Etsuro Fujita
<[email-masked]> wrote:
> I updated the patch that way.  Updated patch attached.  I fixed a bug and
> did a bit of cleanups as well.

Looking this over from a technical point of view, I think it's better
than what you proposed before but I still don't agree with the
approach.  I don't think there is any getting around the fact that
converted whole-row vars are going to result in some warts.  Ashutosh
inserted most of the necessary warts in the original partition-wise
join patch, but missed some cases in postgres_fdw; his approach is,
basically, to add the matching warts there.  Your approach is to rip
out all of those warts and insert different ones.  You've simplified
build_tlist_index_other_vars() and add_placeholders_to_joinrel() and
build_joinrel_tlist() to basically what they were before
partition-wise join went in.  On the other hand, RelOptInfo grows
three new fields, set_append_rel_size() ends up more complicated than
it was before when you include the node code added in the
build_childrel_tlist function it calls, build_child_joinrel() has a
different set of complications, and most importantly
create_append_path() and create_merge_append_path() now do surgery on
their input paths that knows specifically about the
converted-whole-row var case.  I would be glad to be rid of the stuff
that you're ripping out, but in terms of complexity, I don't think we
really come out ahead with the stuff you're adding.

I'm also still concerned about the design.  In your old approach, you
were creating the paths with with the wrong target list and then
fixing it when you turned the paths into a plan.  In your new
approach, you're still creating the paths with the wrong target list,
but now you're fixing it when you put an Append or MergeAppend path on
top of them.  I still think it's a bad idea to have anything other
than the correct paths in the target list from the beginning.  For one
thing, what happens if no Append or MergeAppend is ever put on top of
them?  One way that can happen today is partition-wise aggregate,
although I haven't been able to demonstrate a bug, so maybe that's
covered somehow.  But in general, with your approach, any code that
looks at the tlist for a child-join has to know that the tlist is to
be used as-is *except that* ConvertRowtypeExpr may need to be
inserted.  I think that special case could be easy to overlook, and it
seems pretty arbitrary.  A tlist is a list of arbitrary expressions to
be produced; with this patch, we'd end up with the tlist being the
list of expressions to be produced in every case *except for*
child-joins containing whole-row-vars.  I just can't get behind a
strange exception like that.

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 02 Aug 2018 20:12:15 +0900

> against your proposal is sufficient to say we shouldn't go that way.

Agreed.

> good.

Agreed.

> Can't we just do what Ashutosh proposed for now and revisit
> this for v12?

I think that may be possible.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 02 Aug 2018 20:20:03 +0900

(2018/08/02 4:30), Robert Haas wrote:
> three new fields,

Sorry, I forgot to remove one of the fields that isn't needed anymore. 
RelOptInfo needs two new fields, in the new approach.

> set_append_rel_size() ends up more complicated than

The new approach seems to me more localized than what Ashutosh had 
proposed.  One obvious advantage of the new approach is that we no 
longer need changes to postgres_fdw for it to work for partitionwise 
joins, which would apply for any other FDWs, making the FDW authors' 
life easy.

> than the correct paths in the target list from the beginning.

I don't think so; actually, the child's targetlist would not have to be 
the same as the parent's until the child gets in under the parent's 
Append or MergeAppend.  So, I modified the child's target list so as to 
make it easy to join the child relations.

> For one
> covered somehow.

Right.  In the new approach, the targetlist for the topmost child 
scan/join is guaranteed to be the same as that for the topmost parent 
scan/join by the planner work that I added.

> But in general, with your approach, any code that
> seems pretty arbitrary.

I think we can check to see whether the conversion is needed, from the 
flags added to RelOptInfo.

> A tlist is a list of arbitrary expressions to

I have to admit that it's weird to adjust the child's targetlist in that 
case when putting the child under the parent's Append/MergeAppend, but 
IMHO I think that would be much localized.

Thanks for the comments!

Best regards,
Etsuro Fujita

---

From: Robert Haas <[email-masked]>
Date: Thu, 2 Aug 2018 15:19:49 -0400

On Thu, Aug 2, 2018 at 7:20 AM, Etsuro Fujita
<[email-masked]> wrote:
> The new approach seems to me more localized than what Ashutosh had proposed.
> One obvious advantage of the new approach is that we no longer need changes
> to postgres_fdw for it to work for partitionwise joins, which would apply
> for any other FDWs, making the FDW authors' life easy.

Well, I don't know what to say here expect that I don't agree.  I
think Ashutosh's approach is a pretty natural extension of the system
that we have now.  It involves needing to handle converted whole row
vars in some new places, most of which were handled in the original
patch, but postgres_fdw was missed.  Your approach involves changing
the meaning of the target list, but only in narrow corner cases.  I
completely disagree that we can say it's less invasive.  It may indeed
be less work for extension authors, though, though perhaps at the
expense of moving the conversion from the remote server to the local
one.

> I think we can check to see whether the conversion is needed, from the flags
> added to RelOptInfo.

Sure, I don't dispute that it can be made to work.  I just think it's
an ugly kludge.

> case when putting the child under the parent's Append/MergeAppend, but IMHO
> I think that would be much localized.

Yeah, I just don't agree at all.

Does anyone else want to weigh in on this?  It seems to me that there
are several people who are quite willing to complain about the fact
that this hasn't been fixed, but not willing to express an opinion
about the shape of the fix.  Either the RMT needs to take executive
action, or we need some more votes.

---

From: Andres Freund <[email-masked]>
Date: Thu, 2 Aug 2018 12:30:46 -0700

Hi,

> action, or we need some more votes.

(taking my RMT hat off, as it's not been coordinated)

Well, I think it's enough in the weeds that not too many people are
going to have an informed opinion on this, and I'm not sure uninformed
opinions help. But I do think the fact that one of the authors and the
committer are on one side, and Tom seems to tentatively agree, weighs
quite a bit here.  As far as I can tell, it's "just" Etsuro on the other
side - obviously his opinion counts, but it doesn't outweigh others.  I
think if you feel confident, you should be ok just commit the approach
you favor, and I read you as being confident that that's the better
approach.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Aug 2018 16:25:19 -0400

> action, or we need some more votes.

[ reads thread ... ]

Well, my vote is that I've got zero faith in either of these patches.

I do not trust Ashutosh's patch because of the point you noted that it
will kick in on ConvertRowtypeExprs that are not related to partitioning.
It's also got a rather fundamental conceptual (or at least documentation)
error in that it says it's making pull_var_clause do certain things to
all ConvertRowtypeExprs, but that's not what the code actually does.
I think the tension around that has a lot to do with the fact that the
patch went through so many revisions, and I don't have any faith that
it's right even yet.  As you mentioned upthread, this might be insoluble
without some representational change for converted whole-row Vars.

As for Etsuro-san's patch, I don't like that for the same reasons you
gave.  Also, I note that back towards the beginning of July it was
said that that patch depends on the assumption of no Gather below
an Append.  That's broken *now*, not in some hypothetical future.
(See the nearby "FailedAssertion on partprune" thread, wherein we're
trying to fix the planner's handling of exactly such plans.)

What I'm thinking might be a more appropriate thing, at least for
getting v11 out the door, is to refuse to generate partitionwise
joins when any whole-row vars are involved.  (Perhaps that's not
enough to dodge all the problems, though?)

Now, that's a bit of a problem for postgres_fdw, because it seems to
insist on injecting WRVs even when the query text does not require any.
Why is that, and can't we get rid of it?  It's horrid for performance
even without any of these other considerations.  But if we fail to get
rid of that in time for v11, it would mean that postgres_fdw can't
participate in PWJ, which isn't great but I think we could live with it
for now.

BTW, what exactly do we think the production status of PWJ is, anyway?
I notice that upthread it was stated that enable_partitionwise_join
defaults to off, as indeed it still does, and we'd turn it on later
when we'd gotten rid of some memory-hogging problems.  If that hasn't
happened yet (and I don't see any open item about considering enabling
PWJ by default for v11), then I have exactly no hesitation about
lobotomizing PWJ as hard as we need to to mask this problem for v11.
I'd certainly prefer a solution along those lines to either of these
patches.

			regards, tom lane

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 03 Aug 2018 22:18:34 +0900

(2018/08/03 5:25), Tom Lane wrote:
> joins when any whole-row vars are involved.

Agreed.

> (Perhaps that's not

Sorry, I don't understand this.  Could you elaborate on that a bit more?

> lobotomizing PWJ as hard as we need to to mask this problem for v11.

That hasn't happened yet; I think we left that for PG12, IIRC.

Here is a patch for refusing to generate PWJ paths when WRVs are involved:

* We no longer need to handle WRVs, so I've simplified 
build_joinrel_tlist() and setrefs.c to what they were before 
partition-wise join went in, as in the previous patch.

* attr_needed data for each child is used for building child-joins' 
tlists, but I think we can build those by applying 
adjust_appendrel_attrs to the parent's tlists, without attr_needed.  So, 
I've also removed that as in the previous patch.

Maybe I'm missing something, though.

Best regards,
Etsuro Fujita
*** 8389,8416 **** SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * FROM fprt2 WHERE a < 10)
   8 |   | 
  (5 rows)
  
! -- with whole-row reference
  EXPLAIN (COSTS OFF)
!    ->  Append
!          ->  Foreign Scan
!          ->  Foreign Scan
! (7 rows)

--- 8390,8431 ----
   8 |   | 
  (5 rows)
  
! -- with whole-row reference; partitionwise join does not apply
  EXPLAIN (COSTS OFF)
! SELECT t1.wr, t2.wr FROM (SELECT t1 wr, a FROM fprt1 t1 WHERE t1.a % 25 = 0) t1 FULL JOIN (SELECT t2 wr, b FROM fprt2 t2 WHERE t2.b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY 1,2;
!                        QUERY PLAN                       
! --------------------------------------------------------
   Sort
     Sort Key: ((t1.*)::fprt1), ((t2.*)::fprt2)
!    ->  Hash Full Join
!          Hash Cond: (t1.a = t2.b)
!          ->  Append
!                ->  Foreign Scan on ftprt1_p1 t1
!                ->  Foreign Scan on ftprt1_p2 t1_1
!          ->  Hash
!                ->  Append
!                      ->  Foreign Scan on ftprt2_p1 t2
!                      ->  Foreign Scan on ftprt2_p2 t2_1
! (11 rows)

--- 8489,8522 ----
       |        | 475 | t2_phv
  (14 rows)
  
+ -- test FOR UPDATE; partitionwise join does not apply
+ EXPLAIN (COSTS OFF)
+ (12 rows)
*** 2281,2290 **** EXPLAIN (COSTS OFF)
  SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * FROM fprt2 WHERE a < 10) t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a < 10 ORDER BY 1,2,3;
  SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * FROM fprt2 WHERE a < 10) t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a < 10 ORDER BY 1,2,3;
  
! -- with whole-row reference
  EXPLAIN (COSTS OFF)
! SELECT t1,t2 FROM fprt1 t1 JOIN fprt2 t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a % 25 =0 ORDER BY 1,2;
! SELECT t1,t2 FROM fprt1 t1 JOIN fprt2 t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a % 25 =0 ORDER BY 1,2;
  
  -- join with lateral reference
  EXPLAIN (COSTS OFF)
--- 2282,2291 ----
  SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * FROM fprt2 WHERE a < 10) t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a < 10 ORDER BY 1,2,3;
  SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * FROM fprt2 WHERE a < 10) t2 ON (t1.a = t2.b and t1.b = t2.a) WHERE t1.a < 10 ORDER BY 1,2,3;
  
! -- with whole-row reference; partitionwise join does not apply
  EXPLAIN (COSTS OFF)
! SELECT t1.wr, t2.wr FROM (SELECT t1 wr, a FROM fprt1 t1 WHERE t1.a % 25 = 0) t1 FULL JOIN (SELECT t2 wr, b FROM fprt2 t2 WHERE t2.b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY 1,2;
! SELECT t1.wr, t2.wr FROM (SELECT t1 wr, a FROM fprt1 t1 WHERE t1.a % 25 = 0) t1 FULL JOIN (SELECT t2 wr, b FROM fprt2 t2 WHERE t2.b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY 1,2;

+ -- test FOR UPDATE; partitionwise join does not apply
+ EXPLAIN (COSTS OFF)
+ SELECT t1.a, t2.b FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a = t2.b) WHERE t1.a % 25 = 0 ORDER BY 1,2 FOR UPDATE OF t1;
+ SELECT t1.a, t2.b FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a = t2.b) WHERE t1.a % 25 = 0 ORDER BY 1,2 FOR UPDATE OF t1;
+ 
  RESET enable_partitionwise_join;

*** 2359,2364 **** _outRelOptInfo(StringInfo str, const RelOptInfo *node)
--- 2359,2365 ----
--- 910,926 ----
  	}
  
  	/*
+ 	 * If this is a partitioned baserel, set the consider_partitionwise_join
+ 	 * flag.  Currently, we only consider partitionwise joins with the baserel
+ 	 * if its targetlist doesn't contain a whole-row Var.
+ 	 */
+ 	if (enable_partitionwise_join &&
+ 		rel->reloptkind == RELOPT_BASEREL &&
+ 		rte->relkind == RELKIND_PARTITIONED_TABLE &&
+ 		rel->attr_needed[InvalidAttrNumber - rel->min_attr] == NULL)
+ 		rel->consider_partitionwise_join = true;
+ 
+ 	/*
  	 * Initialize to compute size estimates for whole append relation.
  	 *
  	 * We handle width estimates by weighting the widths of different child
***************
*** 956,1013 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  		childrel = find_base_rel(root, childRTindex);
  		Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL);
  
- 		if (rel->part_scheme)
- 		{
- 			AttrNumber	attno;
- 
- 			/*
  		 * NB: the resulting childrel->reltarget->exprs may contain arbitrary
  		 * expressions, which otherwise would not occur in a rel's targetlist.
--- 967,979 ----
  
  		/*
! 		 * We have to copy the parent's targetlist to the child, with
! 		 * appropriate substitution of variables.
! 		 *
! 		 * Note: when considering partitionwise joining, we need the child's
! 		 * targetlist in case it falls on nullable side in a child join even
! 		 * if the child is deemed empty.
  		 *
  		 * NB: the resulting childrel->reltarget->exprs may contain arbitrary
  		 * expressions, which otherwise would not occur in a rel's targetlist.
***************
*** 1181,1186 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
--- 1147,1168 ----
  								   1, &appinfo);
  
  		/*
+ 		 * Note: we could compute appropriate attr_needed data for the child's
+ 		 * variables, by transforming the parent's attr_needed through the
+ 		 * translated_vars mapping.  However, currently there's no need
+ 		 * because attr_needed is only examined for base relations not
+ 		 * otherrels.  So we just leave the child's attr_needed empty.
+ 		 */
+ 
+ 		/*
+ 		 * If we consider partitionwise joins with the parent rel, do the same
+ 		 * for the partitioned child rel.
+ 		 */
+ 		if (rel->consider_partitionwise_join &&
+ 			childRTE->relkind == RELKIND_PARTITIONED_TABLE)
+ 			childrel->consider_partitionwise_join = true;
+ 
+ 		/*
  		 * If parallelism is allowable for this query in general, see whether
  		 * it's allowable for this childrel in particular.  But if we've
  		 * already decided the appendrel is not parallel-safe as a whole,
***************
*** 3539,3546 **** generate_partitionwise_join_paths(PlannerInfo *root, RelOptInfo *rel)
  	if (!IS_JOIN_REL(rel))
  		return;
  
! 	/* We've nothing to do if the relation is not partitioned. */
! 	if (!IS_PARTITIONED_REL(rel))
  		return;
  
  	/* Guard against stack overflow due to overly deep partition hierarchy. */
--- 3521,3531 ----
  	if (!IS_JOIN_REL(rel))
  		return;
  
! 	/*
! 	 * We've nothing to do if the relation is not partitioned, or
! 	 * partitionwise joins are not allowed for the join relation.
! 	 */
! 	if (!IS_PARTITIONED_REL(rel) || !rel->consider_partitionwise_join)
  		return;
  
  	/* Guard against stack overflow due to overly deep partition hierarchy. */
*** a/src/backend/optimizer/path/joinrels.c
--- b/src/backend/optimizer/path/joinrels.c
***************
*** 1318,1325 **** try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
  	/* Guard against stack overflow due to overly deep partition hierarchy. */
  	check_stack_depth();
  
! 	/* Nothing to do, if the join relation is not partitioned. */
! 	if (!IS_PARTITIONED_REL(joinrel))
  		return;
  
  	/*
--- 1318,1328 ----
  	/* Guard against stack overflow due to overly deep partition hierarchy. */
  	check_stack_depth();
  
! 	/*
! 	 * Nothing to do, if the join relation is not partitioned, or
! 	 * partitionwise joins are not allowed for the join relation.
! 	 */
! 	if (!IS_PARTITIONED_REL(joinrel) || !joinrel->consider_partitionwise_join)
  		return;
  
  	/*
***************
*** 1351,1356 **** try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
--- 1354,1365 ----
  								  joinrel->part_scheme->parttypbyval,
  								  joinrel->boundinfo, rel2->boundinfo));
  
+ 	/*
+ 	 * Partitionwise joins should have been allowed for the joining relations.
+ 	 */
+ 	Assert(rel1->consider_partitionwise_join &&
+ 		   rel2->consider_partitionwise_join);
+ 
  	nparts = joinrel->nparts;
  
  	/*
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***************
*** 3960,3965 **** create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
--- 3960,3967 ----
  	 */
  	if (extra->patype != PARTITIONWISE_AGGREGATE_NONE &&
  		input_rel->part_scheme && input_rel->part_rels &&
+ 		(input_rel->reloptkind == RELOPT_BASEREL ||
+ 		 input_rel->consider_partitionwise_join) &&
  		!IS_DUMMY_REL(input_rel))
  	{
  		/*
***************
*** 6913,6919 **** apply_scanjoin_target_to_paths(PlannerInfo *root,
  	 * projection-capable, that might save a separate Result node, and it also
  	 * is important for partitionwise aggregate.
  	 */
! 	if (rel->part_scheme && rel->part_rels)
  	{
  		int			partition_idx;
  		List	   *live_children = NIL;
--- 6915,6923 ----
  	 * projection-capable, that might save a separate Result node, and it also
  	 * is important for partitionwise aggregate.
  	 */
! 	if (rel->part_scheme && rel->part_rels &&
! 		(rel->reloptkind == RELOPT_BASEREL ||
! 		 rel->consider_partitionwise_join))
  	{
  		int			partition_idx;
  		List	   *live_children = NIL;
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
***************
*** 41,49 **** typedef struct
  	int			num_vars;		/* number of plain Var tlist entries */
  	bool		has_ph_vars;	/* are there PlaceHolderVar entries? */
  	bool		has_non_vars;	/* are there other entries? */
- 	bool		has_conv_whole_rows;	/* are there ConvertRowtypeExpr
- 										 * entries encapsulating a whole-row
- 										 * Var? */
  	tlist_vinfo vars[FLEXIBLE_ARRAY_MEMBER];	/* has num_vars entries */
  } indexed_tlist;
  
--- 41,46 ----
***************

  
  
  	if (node == NULL)
  		return NULL;
--- 2245,2250 ----
***************
  
--- 20,25 ----
***************
  
--- 57,67 ----

  /*
***************
--- 193,199 ----
*** 602,607 **** build_join_rel(PlannerInfo *root,
--- 608,614 ----
--- 667,678 ----
  
+ 	/* Set the consider_partitionwise_join flag. */
+ 	if (IS_PARTITIONED_REL(joinrel) &&
+ 		outer_rel->consider_partitionwise_join &&
+ 		inner_rel->consider_partitionwise_join)
+ 		joinrel->consider_partitionwise_join = true;
+ 
  	/*
  	 * Set estimates of the joinrel's size.
  	 */
***************
*** 732,737 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
--- 745,753 ----
  	/* Only joins between "other" relations land here. */
  	Assert(IS_OTHER_REL(outer_rel) && IS_OTHER_REL(inner_rel));
  
+ 	/* Partitionwise joins should have been allowed for the parent joinrel. */
+ 	Assert(parent_joinrel->consider_partitionwise_join);
+ 
  	joinrel->reloptkind = RELOPT_OTHER_JOINREL;
  	joinrel->relids = bms_union(outer_rel->relids, inner_rel->relids);
  	joinrel->rows = 0;
***************
*** 773,778 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
--- 789,795 ----
  	joinrel->baserestrictcost.per_tuple = 0;
  	joinrel->joininfo = NIL;
  	joinrel->has_eclass_joins = false;
+ 	joinrel->consider_partitionwise_join = false; /* might get changed later */
  	joinrel->top_parent_relids = NULL;
  	joinrel->part_scheme = NULL;
  	joinrel->nparts = 0;
***************
*** 789,802 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
  	/* Compute information relevant to foreign relations. */
  	set_foreign_rel_properties(joinrel, outer_rel, inner_rel);
  
! 	/* Build targetlist */
  
--- 806,818 ----

*** 823,832 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
  								 jointype);
  
  	/* Child joinrel is parallel safe if parent is parallel safe. */
  	joinrel->consider_parallel = parent_joinrel->consider_parallel;
  
- 
  	/* Set estimates of the child-joinrel's size. */
  	set_joinrel_size_estimates(root, joinrel, outer_rel, inner_rel,
  							   sjinfo, restrictlist);
--- 839,851 ----
  	build_joinrel_partition_info(joinrel, outer_rel, inner_rel, restrictlist,
  								 jointype);
  
+ 	/* If so, consider partitionwise joins for that join. */
+ 	if (IS_PARTITIONED_REL(joinrel))
+ 		joinrel->consider_partitionwise_join = true;
+ 
  	/* Child joinrel is parallel safe if parent is parallel safe. */
  	joinrel->consider_parallel = parent_joinrel->consider_parallel;
  
  	/* Set estimates of the child-joinrel's size. */
  	set_joinrel_size_estimates(root, joinrel, outer_rel, inner_rel,
  							   sjinfo, restrictlist);
***************
*** 895,909 **** static void
  build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
  					RelOptInfo *input_rel)
  {
! 	Relids		relids;
  	ListCell   *vars;
  
--- 914,922 ----

  		/*

--- 932,954 ----
  
  		/*

--- 1750,1775 ----

--- 687,698 ----
  
! 	/* used by partitionwise joins: */
! 	bool		consider_partitionwise_join;	/* consider partitionwise
! 												 * join paths? (if
! 												 * partitioned rel) */
! 	Relids		top_parent_relids;	/* Relids of topmost parents (if "other"
! 									 * rel) */
  
  	/* used for partitioned relations */
  	PartitionScheme part_scheme;	/* Partitioning scheme. */
*** a/src/test/regress/expected/partition_aggregate.out
--- b/src/test/regress/expected/partition_aggregate.out
***************
*** 535,540 **** SELECT t1.y, sum(t1.x), count(*) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2
--- 535,572 ----
  (6 rows)
  
  RESET enable_hashagg;
+ -- Check with whole-row reference; partitionwise aggregation does not apply
+ EXPLAIN (COSTS OFF)
+ SELECT t1.x, sum(t1.y), count(t1) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
+                          QUERY PLAN                          
+ -------------------------------------------------------------
+  Sort
+    Sort Key: t1.x, (sum(t1.y)), (count(((t1.*)::pagg_tab1)))
+    ->  HashAggregate
+          Group Key: t1.x
+          ->  Hash Join
+                Hash Cond: (t1.x = t2.y)
+                ->  Append
+                      ->  Seq Scan on pagg_tab1_p1 t1
+                      ->  Seq Scan on pagg_tab1_p2 t1_1
+                      ->  Seq Scan on pagg_tab1_p3 t1_2
+                ->  Hash
+                      ->  Append
+                            ->  Seq Scan on pagg_tab2_p1 t2
+                            ->  Seq Scan on pagg_tab2_p2 t2_1
+                            ->  Seq Scan on pagg_tab2_p3 t2_2
+ (15 rows)
+ 
+ SELECT t1.x, sum(t1.y), count(t1) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
+  x  | sum  | count 
+ ----+------+-------
+   0 |  500 |   100
+   6 | 1100 |   100
+  12 |  700 |   100
+  18 | 1300 |   100
+  24 |  900 |   100
+ (5 rows)
+ 
  -- Check with LEFT/RIGHT/FULL OUTER JOINs which produces NULL values for
  -- aggregation
  -- LEFT JOIN, should produce partial partitionwise aggregation plan as
*** a/src/test/regress/expected/partition_join.out
--- b/src/test/regress/expected/partition_join.out
***************
*** 69,94 **** SELECT t1, t2 FROM prt1 t1 LEFT JOIN prt2 t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER
  --------------------------------------------------
   Sort
     Sort Key: t1.a, t2.b
!    ->  Append
!          ->  Hash Right Join
!                Hash Cond: (t2.b = t1.a)
                 ->  Seq Scan on prt2_p1 t2
!                ->  Hash
                       ->  Seq Scan on prt1_p1 t1
                             Filter: (b = 0)
-          ->  Hash Right Join
-                Hash Cond: (t2_1.b = t1_1.a)
-                ->  Seq Scan on prt2_p2 t2_1
-                ->  Hash
                       ->  Seq Scan on prt1_p2 t1_1
                             Filter: (b = 0)
-          ->  Hash Right Join
-                Hash Cond: (t2_2.b = t1_2.a)
-                ->  Seq Scan on prt2_p3 t2_2
-                ->  Hash
                       ->  Seq Scan on prt1_p3 t1_2
                             Filter: (b = 0)
! (21 rows)
  
  SELECT t1, t2 FROM prt1 t1 LEFT JOIN prt2 t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
        t1      |      t2      
--- 69,89 ----
  --------------------------------------------------
   Sort
     Sort Key: t1.a, t2.b
!    ->  Hash Right Join
!          Hash Cond: (t2.b = t1.a)
!          ->  Append
                 ->  Seq Scan on prt2_p1 t2
!                ->  Seq Scan on prt2_p2 t2_1
!                ->  Seq Scan on prt2_p3 t2_2
!          ->  Hash
!                ->  Append
                       ->  Seq Scan on prt1_p1 t1
                             Filter: (b = 0)
                       ->  Seq Scan on prt1_p2 t1_1
                             Filter: (b = 0)
                       ->  Seq Scan on prt1_p3 t1_2
                             Filter: (b = 0)
! (16 rows)
  
  SELECT t1, t2 FROM prt1 t1 LEFT JOIN prt2 t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
        t1      |      t2      
***************
*** 1042,1047 **** SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT *
--- 1037,1076 ----
   400 |    
  (9 rows)
  
+ -- merge join when expression with whole-row reference needs to be sorted;
+ -- partitionwise join does not apply
+ EXPLAIN (COSTS OFF)
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+                                        QUERY PLAN                                        
+ -----------------------------------------------------------------------------------------
+  Merge Join
+    Merge Cond: ((t1.a = t2.b) AND (((((t1.*)::prt1))::text) = ((((t2.*)::prt2))::text)))
+    ->  Sort
+          Sort Key: t1.a, ((((t1.*)::prt1))::text)
+          ->  Result
+                ->  Append
+                      ->  Seq Scan on prt1_p1 t1
+                      ->  Seq Scan on prt1_p2 t1_1
+                      ->  Seq Scan on prt1_p3 t1_2
+    ->  Sort
+          Sort Key: t2.b, ((((t2.*)::prt2))::text)
+          ->  Result
+                ->  Append
+                      ->  Seq Scan on prt2_p1 t2
+                      ->  Seq Scan on prt2_p2 t2_1
+                      ->  Seq Scan on prt2_p3 t2_2
+ (16 rows)
+ 
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+  a  | b  
+ ----+----
+   0 |  0
+   6 |  6
+  12 | 12
+  18 | 18
+  24 | 24
+ (5 rows)
+ 
  RESET enable_hashjoin;
  RESET enable_nestloop;
  --
*** a/src/test/regress/sql/partition_aggregate.sql
--- b/src/test/regress/sql/partition_aggregate.sql
***************
*** 123,128 **** SELECT t1.y, sum(t1.x), count(*) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2
--- 123,133 ----
  SELECT t1.y, sum(t1.x), count(*) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.y HAVING avg(t1.x) > 10 ORDER BY 1, 2, 3;
  RESET enable_hashagg;
  
+ -- Check with whole-row reference; partitionwise aggregation does not apply
+ EXPLAIN (COSTS OFF)
+ SELECT t1.x, sum(t1.y), count(t1) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
+ SELECT t1.x, sum(t1.y), count(t1) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
+ 
  -- Check with LEFT/RIGHT/FULL OUTER JOINs which produces NULL values for
  -- aggregation
  
--- 160,171 ----
  
+ -- merge join when expression with whole-row reference needs to be sorted;
+ -- partitionwise join does not apply
+ EXPLAIN (COSTS OFF)
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+ SELECT t1.a, t2.b FROM prt1 t1, prt2 t2 WHERE t1::text = t2::text AND t1.a = t2.b ORDER BY t1.a;
+ 
  RESET enable_hashjoin;
  RESET enable_nestloop;

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 03 Aug 2018 22:28:08 +0900

> I've also removed that as in the previous patch.

One thing to add: as for the latter, we don't need the changes to 
placeholder.c either, so I've also simplified that to what they were 
before partition-wise join went in.

Best regards,
Etsuro Fujita

---

From: Robert Haas <[email-masked]>
Date: Fri, 3 Aug 2018 09:44:52 -0400

> will kick in on ConvertRowtypeExprs that are not related to partitioning.

I don't remember making that point -- can you clarify?

> without some representational change for converted whole-row Vars.

Insoluble is a strong word....

> enough to dodge all the problems, though?)

It's enough to dodge the problem being discussed on this thread.

> participate in PWJ, which isn't great but I think we could live with it

I don't quite know what you mean here -- postgres_fdw does use
whole-row vars for EPQ handling, which may be what you're thinking
about.

> I'd certainly prefer a solution along those lines to either of these

Yeah, that hasn't been done yet.  Partition-wise join probably needs a
good bit of work in a number of areas to do all the things that people
will want it to do -- see the thread on advanced partition-matching,
which is an improvement but still doesn't cover every case that could
come up.  In terms of memory usage, I think that we need some
discussion of the approach that should be taken.  I see a couple of
possible things we could do, not necessarily mutually exclusive.

1. We could try to avoid translating all of the parent's data
structures for every child RelOptInfo, either by rejiggering things so
that the child doesn't need all of that data, or by making it able to
use the parent's copy of the data.

2. We could try to recycle memory more quickly.  For example, if we're
planning a partition-wise join of A-B-C-D, first plan paths for
A1-B1-C1-D1 (and all proper subsets of those rels), then throw away
most of the memory, then plan paths for A2-B2-C2-D2, etc.

3. We could generate paths for on "representative" children (e.g. the
biggest ones) and use that to estimate the cost of the partition-wise
plan.  If that plan is selected, then go back and figure out real
paths for the other children.

All of these things help in different ways, and Ashutosh had code for
some version of all of them at various points, but the problem is
quite difficult.  If you have three tables with 1000 partitions each,
then without partition-wise join you need 3000 (partitions) + 3
(baserels) + 3 (level-2 joinrels) + 1 (level-3 joinrel) RelOptInfos.
If you do a partition-wise join, then you get 3000 level-2
child-joinrels and 1000 level-3 child joinrels.  Those RelOptInfo
structures, and the paths attached to them, cannot help but take up
memory.  Perhaps that's OK, and we ought to just say "well, if you
want better plans, you're going to have to allow more memory for
planning".  If not, we have to decide how hard we want to work in
which areas and how good the result needs to be in order to have this
turned on by difficult.

Honestly, I'm pretty impressed that we have added not one but two
members to the RelOptKind enum without as little collateral damage as
there has been.  This issue is annoying and the discussion has gone on
longer than anyone would probably prefer, but it's really pretty minor
in the grand scheme of things, at least IMHO.

---

From: Tom Lane <[email-masked]>
Date: Fri, 03 Aug 2018 10:07:01 -0400

Robert Haas <[email-masked]> writes:
>> Why is that, and can't we get rid of it?

> whole-row vars for EPQ handling, which may be what you're thinking

As far as I can see from the example that started this thread,
postgres_fdw injects WRVs into a PWJ whether or not the query involves
FOR UPDATE; that's why this bug is reproducible in a query without FOR
UPDATE.  But we shouldn't need any EPQ support in that case.

> there has been.

Color me a bit more skeptical about the bug density in that, given
that enable_partitionwise_join is off by default; that means you're
not getting a lot of testing.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Fri, 3 Aug 2018 10:14:04 -0400

> UPDATE.  But we shouldn't need any EPQ support in that case.

I might be missing something, but the original reproducer involves a
FOR UPDATE clause, and the expected regression test output in
postgres_fdw.out appears to show a whole-row var in the foreign scan's
target list only in those examples where a locking clause is present.

> not getting a lot of testing.

You're hard to please.

---

From: Tom Lane <[email-masked]>
Date: Fri, 03 Aug 2018 10:36:08 -0400

> target list only in those examples where a locking clause is present.

Oh, sigh, never mind that --- I was thinking that only the first of
the two example queries involved FOR UPDATE, but they both do.
So no mystery there after all.

Anyway, what I'm basically suggesting is that we just disable support for
PWJ in the problematic cases in v11.  As long as PWJ isn't even on by
default, that's not much of a loss.  Obviously we'll want to fix it in the
future, but the hour grows late for v11, and I think either of these
patches would need quite a bit more work to be committable.

> I don't remember making that point -- can you clarify?

I'm looking at
https://www.postgresql.org/message-id/CA%2BTgmoZSaKq-fYALn5jf6c_X3%3D%3DRb2s8eqLDwGpV%3DLNNhTXYwg%40mail.gmail.com
wherein you said

> this bug; it is refactoring or improvement work best left to a future

I agree with all of that except your conclusion that it's unnecessary
to do it to fix this bug.  I find that entirely unsupported and over-
optimistic, especially in view of the number of iterations Ashutosh
went through trying to fix the fallout from not making a clear
distinction.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Fri, 3 Aug 2018 13:23:29 -0400

> patches would need quite a bit more work to be committable.

That's not my first choice, but I'm OK with it.

> went through trying to fix the fallout from not making a clear

I don't think that the question of how a converted whole-row expr can
really make a difference between being able to fix this bug and not
being able to fix this bug.  It's just a representational choice.  If
we decide that a converted whole-row expr is represented as
ConvertRowTypeExpr on top of a Var, then we have to check for that.
If we revise the representation to use a Var node directly, or to use
a new WholeRowVar node, then we'll just need to check for those things
instead.  I think that the shape of the fix itself does not change.
Moreover, it's pretty much a 1:1 mapping.  It's not like, say,
replacing plans with paths in the whole upper half of the planner,
where the representation is different enough that things that would
have been very difficult to manage in the old representation become
relatively simple in the new one.  It's basically just a question of
how you spell it.  Writing a test for ConvertRowTypeExpr atop Var and
using it a bunch of places is not beautiful, and it's certainly going
to be marginally slower than a direct test for WholeRowVar or some
other representation that bundles the whole thing into one node, but I
think it should work just fine, and the representation can be revised
later in a separate patch once we agree on an approach.

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 06 Aug 2018 21:30:24 +0900

*** 3960,3965 **** create_ordinary_grouping_paths(PlannerInfo *root, 
RelOptInfo *input_rel,
      * projection-capable, that might save a separate Result node, and 
it also
      * projection-capable, that might save a separate Result node, and 
it also

In the above I used the test whether the relation's reloptkind is 
RELOPT_BASEREL or not, but I noticed that I had overlooked the case of a 
multi-level partitioned table.  So I fixed that and added regression 
test cases for that.  I also revised comments a bit.  Attached is an 
updated version of the patch.

Best regards,
Etsuro Fujita

  

  

  

  

  	/*
+ 	 * flag; currently, we only consider partitionwise joins with the baserel
*** 956,1009 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  
- 		if (rel->part_scheme)
- 		{
- 			AttrNumber	attno;
- 
- 			/*
  		 * Copy/Modify targetlist. Even if this child is deemed empty, we need
  		 * its targetlist in case it falls on nullable side in a child-join
--- 967,972 ----
--- 1144,1165 ----
  								   1, &appinfo);
  
  		/*
+ 		 * for partitioned child rels.

--- 3518,3528 ----
  	if (!IS_JOIN_REL(rel))
  		return;
  
! 	/*

  
  	/*
--- 1318,1328 ----
  
! 	/*
  
  	/*
***************
  
+ 	/*
  
  	/*
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***************
*** 3955,3965 **** create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
  	 * If this is the topmost grouping relation or if the parent relation is
  	 * doing some form of partitionwise aggregation, then we may be able to do
  	 * it at this level also.  However, if the input relation is not
! 	 * partitioned, partitionwise aggregate is impossible, and if it is dummy,
  	 * partitionwise aggregate is pointless.
  	 */
  	if (extra->patype != PARTITIONWISE_AGGREGATE_NONE &&
  		input_rel->part_scheme && input_rel->part_rels &&
  		!IS_DUMMY_REL(input_rel))
  	{
  		/*
--- 3955,3967 ----
  	 * If this is the topmost grouping relation or if the parent relation is
  	 * doing some form of partitionwise aggregation, then we may be able to do
  	 * it at this level also.  However, if the input relation is not
! 	 * partitioned or if we are not allowed to generate Append paths for it,
! 	 * partitionwise aggregate is impossible, and if it is dummy,
  	 * partitionwise aggregate is pointless.
  	 */
  	if (extra->patype != PARTITIONWISE_AGGREGATE_NONE &&
  		input_rel->part_scheme && input_rel->part_rels &&
+ 		(IS_SIMPLE_REL(input_rel) || input_rel->consider_partitionwise_join) &&
  		!IS_DUMMY_REL(input_rel))
  	{
  		/*
***************
*** 6908,6919 **** apply_scanjoin_target_to_paths(PlannerInfo *root,
  							  scanjoin_targets_contain_srfs);
  
  	/*
! 	 * If the relation is partitioned, recursively apply the same changes to
! 	 * all partitions and generate new Append paths.  Since Append is not
! 	 * projection-capable, that might save a separate Result node, and it also
! 	 * is important for partitionwise aggregate.
  	 */
! 	if (rel->part_scheme && rel->part_rels)
  	{
  		int			partition_idx;
  		List	   *live_children = NIL;
--- 6910,6923 ----
  							  scanjoin_targets_contain_srfs);
  
  	/*
! 	 * If the relation is partitioned and we are allowed to generate Append
! 	 * paths for it, recursively apply the same changes to all partitions and
! 	 * generate new Append paths.  Since Append is not projection-capable,
! 	 * that might save a separate Result node, and it also is important for
! 	 * partitionwise aggregate.
  	 */
! 	if (rel->part_scheme && rel->part_rels &&
! 		(IS_SIMPLE_REL(rel) || rel->consider_partitionwise_join))
  	{
  		int			partition_idx;
  		List	   *live_children = NIL;
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
***************
*** 41,49 **** typedef struct
  	int			num_vars;		/* number of plain Var tlist entries */
  	bool		has_ph_vars;	/* are there PlaceHolderVar entries? */
  	bool		has_non_vars;	/* are there other entries? */
- 	bool		has_conv_whole_rows;	/* are there ConvertRowtypeExpr
- 										 * entries encapsulating a whole-row
- 										 * Var? */
  	tlist_vinfo vars[FLEXIBLE_ARRAY_MEMBER];	/* has num_vars entries */
  } indexed_tlist;
  
--- 41,46 ----
***************

  
  
  	if (node == NULL)
  		return NULL;
--- 2245,2250 ----
***************
  
--- 20,25 ----
***************

  /*
***************
*** 661,666 **** build_join_rel(PlannerInfo *root,
--- 668,682 ----
  								 sjinfo->jointype);
  
  	/*
+ 	 * Set the consider_partitionwise_join flag if this joinrel is partitioned
+ 	 * and partitionwise joins are allowed for the outer and inner rels.
+ 	 */
--- 748,756 ----
  
--- 792,798 ----
  
! 	/* Build targetlist */
  
--- 809,821 ----

  
  
- 
--- 842,854 ----

  
--- 917,925 ----

  		/*

--- 935,957 ----
  
  		/*

--- 1753,1778 ----

  
  
*** 449,454 **** SELECT t1.x, sum(t1.y), count(*) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2
--- 449,486 ----
   24 |  900 |   100
  (5 rows)
  
+ -- Check with whole-row reference; partitionwise aggregation does not apply
+ EXPLAIN (COSTS OFF)
+ SELECT t1.x, sum(t1.y), count(t1) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
+                          QUERY PLAN                          
+ -------------------------------------------------------------
+  Sort
+    Sort Key: t1.x, (sum(t1.y)), (count(((t1.*)::pagg_tab1)))
+    ->  HashAggregate
+          Group Key: t1.x
+          ->  Hash Join
+                Hash Cond: (t1.x = t2.y)
+                ->  Append
+                      ->  Seq Scan on pagg_tab1_p1 t1
+                      ->  Seq Scan on pagg_tab1_p2 t1_1
+                      ->  Seq Scan on pagg_tab1_p3 t1_2
+                ->  Hash
+                      ->  Append
+                            ->  Seq Scan on pagg_tab2_p1 t2
+                            ->  Seq Scan on pagg_tab2_p2 t2_1
+                            ->  Seq Scan on pagg_tab2_p3 t2_2
+ (15 rows)
+ 
+ SELECT t1.x, sum(t1.y), count(t1) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
+  x  | sum  | count 
+ ----+------+-------
+   0 |  500 |   100
+   6 | 1100 |   100
+  12 |  700 |   100
+  18 | 1300 |   100
+  24 |  900 |   100
+ (5 rows)
+ 
  -- GROUP BY having other matching key
  EXPLAIN (COSTS OFF)
  SELECT t2.y, sum(t1.y), count(*) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t2.y ORDER BY 1, 2, 3;
***************
*** 1062,1067 **** SELECT a, sum(b), count(*) FROM pagg_tab_ml GROUP BY a HAVING avg(b) < 3 ORDER B
--- 1094,1151 ----
   22 | 2000 |  1000
  (9 rows)
  
+ -- Check with whole-row reference
+ EXPLAIN (COSTS OFF)
+ SELECT a, sum(b), count(pagg_tab_ml) FROM pagg_tab_ml GROUP BY a HAVING avg(b) < 3 ORDER BY 1, 2, 3;
+                                             QUERY PLAN                                             
+ ---------------------------------------------------------------------------------------------------
+  Sort
+    Sort Key: pagg_tab_ml_p1.a, (sum(pagg_tab_ml_p1.b)), (count(((pagg_tab_ml_p1.*)::pagg_tab_ml)))
+    ->  Append
+          ->  HashAggregate
+                Group Key: pagg_tab_ml_p1.a
+                Filter: (avg(pagg_tab_ml_p1.b) < '3'::numeric)
+                ->  Seq Scan on pagg_tab_ml_p1
+          ->  Finalize GroupAggregate
+                Group Key: pagg_tab_ml_p2_s1.a
+                Filter: (avg(pagg_tab_ml_p2_s1.b) < '3'::numeric)
+                ->  Sort
+                      Sort Key: pagg_tab_ml_p2_s1.a
+                      ->  Append
+                            ->  Partial HashAggregate
+                                  Group Key: pagg_tab_ml_p2_s1.a
+                                  ->  Seq Scan on pagg_tab_ml_p2_s1
+                            ->  Partial HashAggregate
+                                  Group Key: pagg_tab_ml_p2_s2.a
+                                  ->  Seq Scan on pagg_tab_ml_p2_s2
+          ->  Finalize GroupAggregate
+                Group Key: pagg_tab_ml_p3_s1.a
+                Filter: (avg(pagg_tab_ml_p3_s1.b) < '3'::numeric)
+                ->  Sort
+                      Sort Key: pagg_tab_ml_p3_s1.a
+                      ->  Append
+                            ->  Partial HashAggregate
+                                  Group Key: pagg_tab_ml_p3_s1.a
+                                  ->  Seq Scan on pagg_tab_ml_p3_s1
+                            ->  Partial HashAggregate
+                                  Group Key: pagg_tab_ml_p3_s2.a
+                                  ->  Seq Scan on pagg_tab_ml_p3_s2
+ (31 rows)
+ 
+ SELECT a, sum(b), count(pagg_tab_ml) FROM pagg_tab_ml GROUP BY a HAVING avg(b) < 3 ORDER BY 1, 2, 3;
+  a  | sum  | count 
+ ----+------+-------
+   0 |    0 |  1000
+   1 | 1000 |  1000
+   2 | 2000 |  1000
+  10 |    0 |  1000
+  11 | 1000 |  1000
+  12 | 2000 |  1000
+  20 |    0 |  1000
+  21 | 1000 |  1000
+  22 | 2000 |  1000
+ (9 rows)
+ 
  -- Partial aggregation at all levels as GROUP BY clause does not match with
  -- PARTITION KEY
  EXPLAIN (COSTS OFF)
*** a/src/test/regress/expected/partition_join.out
--- b/src/test/regress/expected/partition_join.out
***************
*** 62,94 **** SELECT t1.a, t1.c, t2.b, t2.c FROM prt1 t1, prt2 t2 WHERE t1.a = t2.b AND t1.b =
   450 | 0450 | 450 | 0450
  (4 rows)
  
! -- left outer join, with whole-row reference
  EXPLAIN (COSTS OFF)
  
--- 62,89 ----
   450 | 0450 | 450 | 0450
  (4 rows)
  
! -- left outer join, with whole-row reference; partitionwise join does not apply
  EXPLAIN (COSTS OFF)

*** 1722,1727 **** SELECT t1.a, t1.c, t2.b, t2.c FROM (SELECT * FROM prt1_l WHERE a = 1 AND a = 2)
--- 1751,1792 ----
                 One-Time Filter: false
  (11 rows)
  
+ -- Check that apply_scanjoin_target_to_paths() works for case with
+ -- whole-row Var referencing the multi-level partitioned table
+ EXPLAIN (COSTS OFF)
+ SELECT t1.tableoid::regclass, t1, t1.a, t1.c FROM prt1_l t1 WHERE t1.b = 0 ORDER BY t1.a;
+                 QUERY PLAN                 
+ -------------------------------------------
+  Sort
+    Sort Key: t1.a
+    ->  Append
+          ->  Seq Scan on prt1_l_p1 t1
+                Filter: (b = 0)
+          ->  Seq Scan on prt1_l_p2_p1 t1_1
+                Filter: (b = 0)
+          ->  Seq Scan on prt1_l_p2_p2 t1_2
+                Filter: (b = 0)
+          ->  Seq Scan on prt1_l_p3_p1 t1_3
+                Filter: (b = 0)
+ (11 rows)
+ 
+ SELECT t1.tableoid::regclass, t1, t1.a, t1.c FROM prt1_l t1 WHERE t1.b = 0 ORDER BY t1.a;
+    tableoid   |      t1      |  a  |  c   
+ --------------+--------------+-----+------
+  prt1_l_p1    | (0,0,0000)   |   0 | 0000
+  prt1_l_p1    | (50,0,0002)  |  50 | 0002
+  prt1_l_p1    | (100,0,0000) | 100 | 0000
+  prt1_l_p1    | (150,0,0002) | 150 | 0002
+  prt1_l_p1    | (200,0,0000) | 200 | 0000
+  prt1_l_p2_p2 | (250,0,0002) | 250 | 0002
+  prt1_l_p2_p1 | (300,0,0000) | 300 | 0000
+  prt1_l_p2_p2 | (350,0,0002) | 350 | 0002
+  prt1_l_p2_p1 | (400,0,0000) | 400 | 0000
+  prt1_l_p2_p2 | (450,0,0002) | 450 | 0002
+  prt1_l_p3_p1 | (500,0,0000) | 500 | 0000
+  prt1_l_p3_p1 | (550,0,0002) | 550 | 0002
+ (12 rows)
+ 
  --
  -- negative testcases
  --
*** a/src/test/regress/sql/partition_aggregate.sql
--- b/src/test/regress/sql/partition_aggregate.sql
***************
*** 111,116 **** EXPLAIN (COSTS OFF)
--- 111,121 ----
  SELECT t1.x, sum(t1.y), count(*) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
  SELECT t1.x, sum(t1.y), count(*) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
  
+ -- Check with whole-row reference; partitionwise aggregation does not apply
+ EXPLAIN (COSTS OFF)
+ SELECT t1.x, sum(t1.y), count(t1) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
+ SELECT t1.x, sum(t1.y), count(t1) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t1.x ORDER BY 1, 2, 3;
+ 
  -- GROUP BY having other matching key
  EXPLAIN (COSTS OFF)
  SELECT t2.y, sum(t1.y), count(*) FROM pagg_tab1 t1, pagg_tab2 t2 WHERE t1.x = t2.y GROUP BY t2.y ORDER BY 1, 2, 3;
***************
*** 230,235 **** EXPLAIN (COSTS OFF)
--- 235,245 ----
  SELECT a, sum(b), count(*) FROM pagg_tab_ml GROUP BY a HAVING avg(b) < 3 ORDER BY 1, 2, 3;
  SELECT a, sum(b), count(*) FROM pagg_tab_ml GROUP BY a HAVING avg(b) < 3 ORDER BY 1, 2, 3;
  
+ -- Check with whole-row reference
+ EXPLAIN (COSTS OFF)
+ SELECT a, sum(b), count(pagg_tab_ml) FROM pagg_tab_ml GROUP BY a HAVING avg(b) < 3 ORDER BY 1, 2, 3;
+ SELECT a, sum(b), count(pagg_tab_ml) FROM pagg_tab_ml GROUP BY a HAVING avg(b) < 3 ORDER BY 1, 2, 3;
+ 
  -- Partial aggregation at all levels as GROUP BY clause does not match with
  -- PARTITION KEY
  EXPLAIN (COSTS OFF)
*** a/src/test/regress/sql/partition_join.sql
--- b/src/test/regress/sql/partition_join.sql
***************
*** 34,40 **** EXPLAIN (COSTS OFF)
  SELECT t1.a, t1.c, t2.b, t2.c FROM prt1 t1, prt2 t2 WHERE t1.a = t2.b AND t1.b = 0 ORDER BY t1.a, t2.b;
  SELECT t1.a, t1.c, t2.b, t2.c FROM prt1 t1, prt2 t2 WHERE t1.a = t2.b AND t1.b = 0 ORDER BY t1.a, t2.b;
  
! -- left outer join, with whole-row reference
  EXPLAIN (COSTS OFF)
  SELECT t1, t2 FROM prt1 t1 LEFT JOIN prt2 t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
  SELECT t1, t2 FROM prt1 t1 LEFT JOIN prt2 t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
--- 34,40 ----
  SELECT t1.a, t1.c, t2.b, t2.c FROM prt1 t1, prt2 t2 WHERE t1.a = t2.b AND t1.b = 0 ORDER BY t1.a, t2.b;
  SELECT t1.a, t1.c, t2.b, t2.c FROM prt1 t1, prt2 t2 WHERE t1.a = t2.b AND t1.b = 0 ORDER BY t1.a, t2.b;
  
! -- left outer join, with whole-row reference; partitionwise join does not apply
  EXPLAIN (COSTS OFF)
  SELECT t1, t2 FROM prt1 t1 LEFT JOIN prt2 t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
  SELECT t1, t2 FROM prt1 t1 LEFT JOIN prt2 t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
***************
*** 160,165 **** EXPLAIN (COSTS OFF)
--- 160,171 ----
  SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT * FROM prt2 WHERE b > 250) t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;
  SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT * FROM prt2 WHERE b > 250) t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b;

***************
*** 340,345 **** SELECT * FROM prt1_l t1 LEFT JOIN LATERAL
--- 346,357 ----
  EXPLAIN (COSTS OFF)
  SELECT t1.a, t1.c, t2.b, t2.c FROM (SELECT * FROM prt1_l WHERE a = 1 AND a = 2) t1 RIGHT JOIN prt2_l t2 ON t1.a = t2.b AND t1.b = t2.a AND t1.c = t2.c;
  
+ -- Check that apply_scanjoin_target_to_paths() works for case with
+ -- whole-row Var referencing the multi-level partitioned table
+ EXPLAIN (COSTS OFF)
+ SELECT t1.tableoid::regclass, t1, t1.a, t1.c FROM prt1_l t1 WHERE t1.b = 0 ORDER BY t1.a;
+ SELECT t1.tableoid::regclass, t1, t1.a, t1.c FROM prt1_l t1 WHERE t1.b = 0 ORDER BY t1.a;
+ 
  --
  -- negative testcases
  --

---

From: Robert Haas <[email-masked]>
Date: Sun, 12 Aug 2018 22:57:25 -0400

On Mon, Aug 6, 2018 at 8:30 AM, Etsuro Fujita
<[email-masked]> wrote:
> multi-level partitioned table.  So I fixed that and added regression test
> cases for that.  I also revised comments a bit.  Attached is an updated
> version of the patch.

Maybe this should assert that the inner and outer rels have
consider_partitionwise_join set.  There is an Assert quite a bit
earlier in the function that the parent join have it set, but I think
it might make sense to check the children have it set whenever we set
the flag.

Aside from that I don't really have any suggestions on this.

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 13 Aug 2018 21:32:59 +0900

> it might make sense to check the children have it set whenever we set

Agreed.  Done.

One thing I noticed might be an improvement is to skip 
build_joinrel_partition_info if the given joinrel will be to have 
consider_partitionwise_join=false; in the previous patch, that function 
created the joinrel's partition info such as part_scheme and part_rels 
if the joinrel is considered as partitioned, independently of the flag 
consider_partitionwise_join for it, but if that flag is false, we don't 
generate PWJ paths for the joinrel, so we would not need to create that 
partition info at all.  This would not only avoid unnecessary processing 
in that function, but also make unnecessary the changes I made to 
try_partitionwise_join, generate_partitionwise_join_paths, 
apply_scanjoin_target_to_paths, and create_ordinary_grouping_paths.  So 
I updated the patch that way.  Please find attached an updated version 
of the patch.

Thanks for the review!

Best regards,
Etsuro Fujita

  

  

  

  

*** 2368,2373 **** _outRelOptInfo(StringInfo str, const RelOptInfo *node)
--- 2368,2374 ----
  
  	/*
  
- 		if (rel->part_scheme)
- 		{
- 			AttrNumber	attno;
- 
- 			/*
  
  		/*
*** 3538,3543 **** generate_partitionwise_join_paths(PlannerInfo *root, RelOptInfo *rel)
--- 3517,3525 ----
  	if (!IS_PARTITIONED_REL(rel))
  		return;
  
+ 	/* The relation should have consider_partitionwise_join set. */
+ 	Assert(rel->consider_partitionwise_join);
+ 
  	/* Guard against stack overflow due to overly deep partition hierarchy. */
  	check_stack_depth();
  
*** a/src/backend/optimizer/path/joinrels.c
--- b/src/backend/optimizer/path/joinrels.c
***************
*** 1322,1327 **** try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
--- 1322,1330 ----
  	if (!IS_PARTITIONED_REL(joinrel))
  		return;
  
+ 	/* The join relation should have consider_partitionwise_join set. */
+ 	Assert(joinrel->consider_partitionwise_join);
+ 
  	/*
  	 * Since this join relation is partitioned, all the base relations
  	 * participating in this join must be partitioned and so are all the
***************
*** 1330,1335 **** try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
--- 1333,1342 ----
  	Assert(IS_PARTITIONED_REL(rel1) && IS_PARTITIONED_REL(rel2));
  	Assert(REL_HAS_ALL_PART_PROPS(rel1) && REL_HAS_ALL_PART_PROPS(rel2));
  
+ 	/* The joining relations should have consider_partitionwise_join set. */
  	 * The partition scheme of the join relation should match that of the
  	 * joining relations.
  
--- 41,46 ----
***************

  
  
  	if (node == NULL)
  		return NULL;
--- 2245,2250 ----
***************
  
--- 20,25 ----
***************

  /*
***************
--- 739,747 ----
  
+ 	/* The parent joinrel should have consider_partitionwise_join set. */
--- 783,789 ----
  
! 	/* Build targetlist */
  
--- 800,812 ----

*** 826,832 **** build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
  
- 
--- 836,841 ----
  
--- 904,912 ----

  		/*

--- 922,944 ----
  
  		/*

*** 1626,1641 **** build_joinrel_partition_info(RelOptInfo *joinrel, RelOptInfo *outer_rel,
  
  	/*
  	 * We can only consider this join as an input to further partitionwise
! 	 * joins if (a) the input relations are partitioned, (b) the partition
! 	 * schemes match, and (c) we can identify an equi-join between the
! 	 * partition keys.  Note that if it were possible for
! 	 * have_partkey_equi_join to return different answers for the same joinrel
! 	 * depending on which join ordering we try first, this logic would break.
! 	 * That shouldn't happen, though, because of the way the query planner
! 	 * deduces implied equalities and reorders the joins. Please see
! 	 * optimizer/README for details.
  	 */
  	if (!IS_PARTITIONED_REL(outer_rel) || !IS_PARTITIONED_REL(inner_rel) ||
  		outer_rel->part_scheme != inner_rel->part_scheme ||
  		!have_partkey_equi_join(joinrel, outer_rel, inner_rel,
  								jointype, restrictlist))
--- 1598,1615 ----
  
  	/*
  	 * We can only consider this join as an input to further partitionwise
! 	 * joins if (a) the input relations are partitioned and have
! 	 * consider_partitionwise_join=true, (b) the partition schemes match, and
! 	 * (c) we can identify an equi-join between the partition keys.  Note that
! 	 * if it were possible for have_partkey_equi_join to return different
! 	 * answers for the same joinrel depending on which join ordering we try
! 	 * first, this logic would break.  That shouldn't happen, though, because
! 	 * of the way the query planner deduces implied equalities and reorders
! 	 * the joins.  Please see optimizer/README for details.
  	 */
  	if (!IS_PARTITIONED_REL(outer_rel) || !IS_PARTITIONED_REL(inner_rel) ||
+ 		!outer_rel->consider_partitionwise_join ||
+ 		!inner_rel->consider_partitionwise_join ||
  		outer_rel->part_scheme != inner_rel->part_scheme ||
  		!have_partkey_equi_join(joinrel, outer_rel, inner_rel,
  								jointype, restrictlist))
***************
*** 1687,1692 **** build_joinrel_partition_info(RelOptInfo *joinrel, RelOptInfo *outer_rel,
--- 1661,1672 ----
  	joinrel->part_rels =
  		(RelOptInfo **) palloc0(sizeof(RelOptInfo *) * joinrel->nparts);
  
+ 	/*
+ 	 * Set the consider_partitionwise_join flag.
+ 	 */
+ 	Assert(outer_rel->consider_partitionwise_join);
+ 	Assert(inner_rel->consider_partitionwise_join);
+ 	joinrel->consider_partitionwise_join = true;
  
  	/*
  	 * Construct partition keys for the join.
***************
--- 1748,1773 ----

  

  

  

*** 1765,1770 **** WHERE EXISTS (
--- 1794,1835 ----
                             ->  Seq Scan on int8_tbl int8_tbl_2
  (28 rows)

  

  
  
***************
*** 352,357 **** WHERE EXISTS (
--- 358,369 ----
           LATERAL (SELECT int4_tbl.f1 FROM int8_tbl LIMIT 2) ss
      WHERE prt1_l.c IS NULL);

---

From: Robert Haas <[email-masked]>
Date: Tue, 14 Aug 2018 15:51:10 +0000

On Mon, Aug 13, 2018 at 12:32 PM, Etsuro Fujita
<[email-masked]> wrote:
> created the joinrel's partition info such as part_scheme and part_rels if
> the joinrel is considered as partitioned, independently of the flag
> partition info at all.  This would not only avoid unnecessary processing in
> that function, but also make unnecessary the changes I made to
> apply_scanjoin_target_to_paths, and create_ordinary_grouping_paths.  So I
> updated the patch that way.  Please find attached an updated version of the
> patch.

I guess the question is whether there are (or might be in the future)
other dependencies on part_scheme.  For example, it looks like
partition pruning uses it.  I'm not sure whether partition pruning
supports a plan like:

Append
-> Nested Loop
  -> Seq Scan on p1
  -> Index Scan on q1
<repeat the above for p2/q2 etc.>

If it doesn't, that's just an implementation restriction; somebody
might want to fix things so it works, if it doesn't already.

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Tue, 14 Aug 2018 17:06:31 -0400

> On Aug 14, 2018, at 11:51 AM, Robert Haas <[email-masked]> wrote:
> 

While we (the RMT) are happy to see there has been progress on this thread,
11 Beta 3 was released containing this problem and the time to adequately
test this feature prior to GA release is rapidly dwindling.

The RMT would like to see this prioritized and resolved. At this point we have
considered the option of having partitionwise joins completely disabled in
PostgreSQL 11. This is preferably not the path we want to choose, but this
option is now on the table and we will issue an ultimatum soon if we do not see
further progress.

In the previous discussion, the generally accepted solution for PostgreSQL 11
seemed to be to disable the problematic case and any further work would be for
PostgreSQL 12 only. If something is different, please indicate why ASAP and
work to resolve. The RMT will also conduct some additional technical analysis
as well.

Sincerely,

Alvaro, Andres, Jonathan

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 15 Aug 2018 12:25:58 +0900

> <repeat the above for p2/q2 etc.>

I'm not sure that either, but if a join relation doesn't have 
part_scheme set, it means that that relation is considered as 
non-partitioned, as in the case when enable_partitionwise_join is off, 
so there would be no PWJ paths generated for it, to begin with.  So in 
that case, ISTM that we don't need to worry about that at least for 
partition pruning.

Best regards,
Etsuro Fujita

---

From: Amit Langote <[email-masked]>
Date: Wed, 15 Aug 2018 13:04:55 +0900

On 2018/08/15 12:25, Etsuro Fujita wrote:
> I'm not sure that either, but if a join relation doesn't have part_scheme
> set, it means that that relation is considered as non-partitioned, as in
> the case when enable_partitionwise_join is off, so there would be no PWJ
> paths generated for it, to begin with.  So in that case, ISTM that we
> don't need to worry about that at least for partition pruning.

Fwiw, partition pruning works only for base rels, which applies to both
planning-time pruning (pruning is performed only during base rel size
estimation) and run-time pruning (we'll add pruning info to the Append
plan only if the source AppendPath's parent rel is a base rel).

Thanks,
Amit

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 15 Aug 2018 20:35:55 +0900

> plan only if the source AppendPath's parent rel is a base rel).

Thanks for that, Amit!

I looked into the question for the join or upper rel case, but couldn't 
find any places in the PG11 code where we assume that a join or upper 
rel has non-NULL part_scheme, as described below:

* Both try_partitionwise_join() and generate_partitionwise_join_paths() 
check whether a join rel to be considered has non-NULL part_scheme, 
through the IS_PARTITIONED_REL macro:

#define IS_PARTITIONED_REL(rel) \
     ((rel)->part_scheme && (rel)->boundinfo && (rel)->nparts > 0 && \
      (rel)->part_rels && !(IS_DUMMY_REL(rel)))

If IS_PARTITIONED_REL, the former adds paths to child-joins, and the 
latter builds PWJ paths; but both don't assume non-NULL part_scheme.

* apply_scanjoin_target_to_paths() checks whether the topmost scan/join 
rel has non-NULL part_scheme, and if so, it recursively adjusts all 
partitions; it doesn't assume non-NULL part_scheme.

* create_ordinary_grouping_paths() also checks whether the topmost 
scan/join rel adjusted by apply_scanjoin_target_to_paths() has non-NULL 
part_scheme, and if so, it considers PWA paths; it doesn't assume 
non-NULL part_scheme either.

* add_paths_to_append_rel(), which is called from each of the above (ie, 
generate_partitionwise_join_paths(), apply_scanjoin_target_to_paths(), 
and create_partitionwise_grouping_paths() in 
create_ordinary_grouping_paths() for creating PWJ paths, adjusting PWJ 
paths, and creating PWA paths respectively) also checks whether a rel to 
be considered has non-NULL part_scheme, but never assumes that.  And 
actually, if the rel's part_scheme is NULL, add_paths_to_append_rel() 
wouldn't be called for the rel, because of the reason described above.

Thanks for the comments, Robert!

Best regards,
Etsuro Fujita

---

From: Robert Haas <[email-masked]>
Date: Wed, 15 Aug 2018 10:40:40 -0400

> Thanks for the comments, Robert!

Given the comments from the RMT, and also on general principle, it
seems like we really need to get on with committing something here.
It's my understanding you plan to do that, since it's your patch.
When?

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 16 Aug 2018 12:00:16 +0900

> It's my understanding you plan to do that, since it's your patch.

I plan to do that late next week as I'll go on leave until next Tuesday.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 22 Aug 2018 20:08:57 +0900

> I plan to do that late next week as I'll go on leave until next Tuesday.

I added the commit message.  Please find attached an updated version of 
the patch.  Does that make sense?  If there are not objections, I'll 
push this tomorrow.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 23 Aug 2018 22:00:49 +0900

(2018/08/22 20:08), Etsuro Fujita wrote:
> the patch. Does that make sense? If there are not objections, I'll push
> this tomorrow.

I tried this today, but doing git behind the corporate firewall doesn't 
work.  I don't know the clear cause of that, so I'll investigate that 
tomorrow.

Best regards,
Etsuro Fujita

---

From: Michael Paquier <[email-masked]>
Date: Fri, 24 Aug 2018 11:47:10 +0900

> work.  I don't know the clear cause of that, so I'll investigate that

You may be able to tweak that by using https as origin point or proper
git proxy settings?
--
Michael

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 24 Aug 2018 21:38:48 +0900

> You may be able to tweak that by using https as origin point or proper

Yeah, my proxy settings were not correct.  With the help of my 
colleagues Horiguchi-san and Yamada-san, I corrected them but still 
can't clone the master repository.  Running git with GIT_CURL_VERBOSE 
shows that there is another issue in my terminal environment, so I'm 
trying to resolve that.

Thanks for the advice!

Best regards,
Etsuro Fujita

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Tue, 28 Aug 2018 11:21:47 -0400

> On Aug 24, 2018, at 8:38 AM, Etsuro Fujita <[email-masked]> wrote:
> 
> Yeah, my proxy settings were not correct.  With the help of my colleagues Horiguchi-san and Yamada-san, I corrected them but still can't clone the master repository.  Running git with GIT_CURL_VERBOSE shows that there is another issue in my terminal environment, so I'm trying to resolve that.

Are there any updates on getting this patch committed?

Thanks,

Jonathan

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 29 Aug 2018 18:40:03 +0900

(2018/08/29 0:21), Jonathan S. Katz wrote:
>> On Aug 24, 2018, at 8:38 AM, Etsuro Fujita<[email-masked]>  wrote:

That investigation has shown that the cause is my company firewall, not 
my terminal environment; that firewall has to be configured to allow me 
to access to that repository.  So, I'm asking my company about that.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 30 Aug 2018 20:25:08 +0900

(2018/08/29 18:40), Etsuro Fujita wrote:
>>> On Aug 24, 2018, at 8:38 AM, Etsuro
>>> Fujita<[email-masked]> wrote:

I got the approval from my boss today, so I think that I can have that 
access soon.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 31 Aug 2018 20:54:13 +0900

> I got the approval from my boss today, so I think that I can have that

I fixed typos in the commit message, which Alvaro pointed out off-list, 
and revised the message a little bit.  Also, I reread the patch and 
noticed that the latest version includes now-unnecessary regression test 
cases; those were added to the previous version 
(refuse-pwj-when-wrvs-involved-2.patch in [1]) to check that 
apply_scanjoin_target_to_paths() and create_ordinary_grouping_paths() 
work for cases where whole-row Vars are involved, because I modified 
those functions, but the latest version doesn't touch those functions 
anymore, so those test cases seems unnecessary.  So I removed those (no 
other changes), and committed the patch.

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5B683F60.2070806%40lab.ntt.co.jp

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Fri, 31 Aug 2018 08:30:06 -0400

On 8/31/18 7:54 AM, Etsuro Fujita wrote:
> I fixed typos in the commit message, which Alvaro pointed out
> off-list, and revised the message a little bit.  Also, I reread the
> patch and noticed that the latest version includes now-unnecessary
> regression test cases; those were added to the previous version
> anymore, so those test cases seems unnecessary.  So I removed those
> (no other changes), and committed the patch.

Thank you for taking care of that and for committing the patch. I have
now closed this issues on the open items list.

Jonathan

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 31 Aug 2018 21:40:24 +0900

Thanks!

Best regards,
Etsuro Fujita

---

From: Amit Langote <[email-masked]>
Date: Tue, 2 Oct 2018 16:11:54 +0900

>> now closed this issues on the open items list.

I noticed that the CF entry for this was not closed.  As of this morning,
it's been moved to the next 2018-11 CF:

https://commitfest.postgresql.org/20/1554/

I tried to close it as being committed, but couldn't do so, because I
can't find Fujita-san's name in the list of committers in the CF app's
drop down box that lists all committers.

Thanks,
Amit

---

From: Michael Paquier <[email-masked]>
Date: Tue, 2 Oct 2018 16:45:40 +0900

> drop down box that lists all committers.

Indeed, Fujita-san has been added to the list.  And I switched this CF
entry at the same time.
--
Michael

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 02 Oct 2018 17:20:13 +0900

> Indeed, Fujita-san has been added to the list.  And I switched this CF

Thank you guys!

Best regards,
Etsuro Fujita