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

On Thu, May 10, 2018 at 6:23 PM, Etsuro Fujita
<fujita.etsuro@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.etsuro@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