On Thu, May 10, 2018 at 6:23 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > (2018/05/10 13:04), Ashutosh Bapat wrote: >> >> On Wed, May 9, 2018 at 5:20 PM, Etsuro Fujita >> <fujita.ets...@lab.ntt.co.jp> 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
Description: GNU Zip compressed data