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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

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

postgres_fdw_pwj_error.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Mon, Feb 12, 2018 at 6:00 PM, Rajkumar Raghuwanshi
<[hidden email]> 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);
>
> Attached patch have queries which are throwing mentioned error on running
> make check in contrib/postgres_fdw folder.
>

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


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

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.


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

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

Reply | Threaded
Open this post in threaded view
|

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

Etsuro Fujita
(2018/02/13 21:51), Ashutosh Bapat wrote:

> 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

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

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

Thanks for the analysis!

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

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

Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Wed, Feb 14, 2018 at 2:50 PM, Etsuro Fujita
<[hidden email]> wrote:
>
> 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.

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.

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

Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
In reply to this post by Ashutosh Bapat
On Tue, Feb 13, 2018 at 6:21 PM, Ashutosh Bapat
<[hidden email]> wrote:

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

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

expr_ref_error_pwj.tar.gz (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Etsuro Fujita
(2018/02/20 18:13), Ashutosh Bapat wrote:
> 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.

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

Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Tue, Apr 17, 2018 at 2:05 PM, Etsuro Fujita
<[hidden email]> wrote:

> (2018/02/20 18:13), Ashutosh Bapat wrote:
>>
>> 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.
>
>
> 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.

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

expr_ref_error_pwj_v2.tar.gz (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Etsuro Fujita
(2018/04/17 18:43), Ashutosh Bapat wrote:

> On Tue, Apr 17, 2018 at 2:05 PM, Etsuro Fujita
> <[hidden email]>  wrote:
>> (2018/02/20 18:13), Ashutosh Bapat wrote:
>>>
>>> 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.
>>
>>
>> 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.

Thanks for updating the patch set!  Will review.

Best regards,
Etsuro Fujita

Reply | Threaded
Open this post in threaded view
|

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

Etsuro Fujita
(2018/04/17 19:00), Etsuro Fujita wrote:
> (2018/04/17 18:43), Ashutosh Bapat wrote:
>> Here's updated patch-set.
>
> 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

Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Tue, Apr 24, 2018 at 4:49 PM, Etsuro Fujita
<[hidden email]> wrote:

> (2018/04/17 19:00), Etsuro Fujita wrote:
>>
>> (2018/04/17 18:43), Ashutosh Bapat wrote:
>>>
>>> Here's updated patch-set.
>>
>>
>> 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()?
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.

> 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.
Thanks for the catch. I have updated patch with your suggested fix.

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

expr_ref_error_pwj_v3.tar.gz (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Etsuro Fujita
(2018/04/25 18:51), Ashutosh Bapat wrote:
> On Tue, Apr 24, 2018 at 4:49 PM, Etsuro Fujita
> <[hidden email]>  wrote:
>> o 0001-Handle-ConvertRowtypeExprs-in-pull_vars_clause.patch:

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

Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Thu, Apr 26, 2018 at 5:36 PM, Etsuro Fujita
<[hidden email]> wrote:

> (2018/04/25 18:51), Ashutosh Bapat wrote:
>>
>> On Tue, Apr 24, 2018 at 4:49 PM, Etsuro Fujita
>> <[hidden email]>  wrote:
>>>
>>> o 0001-Handle-ConvertRowtypeExprs-in-pull_vars_clause.patch:
>
>
>>> 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.
>>
>>
>> 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
>
Thanks for the test.

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

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

expr_ref_error_pwj_v4.tar.gz (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Etsuro Fujita
(2018/04/27 14:40), Ashutosh Bapat wrote:
> 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

Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Tue, May 1, 2018 at 5:00 PM, Etsuro Fujita
<[hidden email]> wrote:

> (2018/04/27 14:40), Ashutosh Bapat wrote:
>>
>> 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:

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

>
> To fix this, I think we should skip the deparseColumnRef processing for
> 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
> ConvertRowtypeExprs are correctly deparsed in the remote queries.

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.

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

expr_ref_error_pwj_v5.tar.gz (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Etsuro Fujita
(2018/05/08 16:20), Ashutosh Bapat wrote:

> On Tue, May 1, 2018 at 5:00 PM, Etsuro Fujita
> <[hidden email]>  wrote:
>> 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);

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

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

Thanks for those changes!

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

+1

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

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

> PFA patch-set with the fixes.

Thanks for updating the patch!

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

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:

(2018/04/25 18:51), Ashutosh Bapat wrote:
 > 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.

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

Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Wed, May 9, 2018 at 5:20 PM, Etsuro Fujita
<[hidden email]> wrote:

>
> (2018/04/25 18:51), Ashutosh Bapat wrote:
>> 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.
>
> 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.

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?

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

Reply | Threaded
Open this post in threaded view
|

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

Etsuro Fujita
(2018/05/10 13:04), Ashutosh Bapat wrote:

> On Wed, May 9, 2018 at 5:20 PM, Etsuro Fujita
> <[hidden email]>  wrote:
>> (2018/04/25 18:51), Ashutosh Bapat wrote:
>>> 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.
>>
>> 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.
>
> 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.

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

Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Thu, May 10, 2018 at 6:23 PM, Etsuro Fujita
<[hidden email]> wrote:

> (2018/05/10 13:04), Ashutosh Bapat wrote:
>>
>> On Wed, May 9, 2018 at 5:20 PM, Etsuro Fujita
>> <[hidden email]>  wrote:
>>>
>>> (2018/04/25 18:51), Ashutosh Bapat wrote:
>>>>
>>>> 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.
>>>
>>>
>>> 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.
>>
>>
>> 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.
>
>
> 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.
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 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.
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.

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

expr_ref_error_pwj_v6.tar.gz (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Etsuro Fujita
(2018/05/11 16:17), Ashutosh Bapat wrote:

> On Thu, May 10, 2018 at 6:23 PM, Etsuro Fujita
> <[hidden email]>  wrote:
>> 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.
>
> 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));

I guess you forgot to show the query.

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

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.

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

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

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

Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat
On Fri, May 11, 2018 at 6:31 PM, Etsuro Fujita
<[hidden email]> wrote:
>
> 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;

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

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.

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

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.

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

12
Previous Thread Next Thread