Message-ID: <5AF59415.10309@lab.ntt.co.jp>
Date: Fri, 11 May 2018 22:01:09 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with
 partition
 wise join enabled.
References: 
 <CAKcux6ktu-8tefLWtQuuZBYFaZA83vUzuRd7c1YHC-yEWyYFpg@mail.gmail.com>
 <CAFjFpRfD5=LsOqG1eUf-vMpm9KoEZBBGJo68D9wkUBJxop_emQ@mail.gmail.com>
 <CAFjFpReD2rwzw64T5ZHS9+S8avREuazLu4nnCRAXToUN=WC+-Q@mail.gmail.com>
 <5AD5B1E0.8020404@lab.ntt.co.jp>
 <CAFjFpRcHQ9p0JEpx4eMhuNvD7GbyMYm1TykfCeBrNTdK+mSbgA@mail.gmail.com>
 <5AD5C5A1.1070509@lab.ntt.co.jp> <5ADF12D1.1030406@lab.ntt.co.jp>
 <CAFjFpRennScZ47UwYaE+3SB_KcGVToPmR2dOTEb7T7w2=j-jbQ@mail.gmail.com>
 <5AE1C0D3.5060009@lab.ntt.co.jp>
 <CAFjFpRetD0aPMgw_mhOtJ11AqWJFCTo9dNJ758Zh4SPqgN1qvw@mail.gmail.com>
 <5AE84FBF.3040905@lab.ntt.co.jp>
 <CAFjFpRdSvXxOkSMc=jEHfTVGnCFgziLA7m+onp=ak=Tfa_V1zA@mail.gmail.com>
 <5AF2E09F.9060208@lab.ntt.co.jp>
 <CAFjFpRcFucgvrzEFNi10g00-va640poSmJDmBrZNfFarB6eP-Q@mail.gmail.com>
 <5AF440C2.3040805@lab.ntt.co.jp>
 <CAFjFpRczEuG9b0_j_qve2JAi-HHqS9_m1_yK5QVQa5=oDr5mJA@mail.gmail.com>
In-Reply-To: 
 <CAFjFpRczEuG9b0_j_qve2JAi-HHqS9_m1_yK5QVQa5=oDr5mJA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
To: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Precedence: bulk

(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