Re: Expression errors with "FOR UPDATE" and postgres_fdw with partitionwise join enabled. - Mailing list pgsql-hackers

(2018/05/11 16:17), Ashutosh Bapat wrote:
> On Thu, May 10, 2018 at 6:23 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp>  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