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