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

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