Message-ID: <5AF440C2.3040805@lab.ntt.co.jp>
Date: Thu, 10 May 2018 21:53:22 +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>
In-Reply-To: 
 <CAFjFpRcFucgvrzEFNi10g00-va640poSmJDmBrZNfFarB6eP-Q@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/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.

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