Message-ID: <5AFE81CC.8000508@lab.ntt.co.jp>
Date: Fri, 18 May 2018 16:33:32 +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>
 <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>
 <5AF59415.10309@lab.ntt.co.jp>
 <CAFjFpRfkafBFJSEEkGwzLf=-y9Q8TUEGfpR=+K7-zCOw80R10w@mail.gmail.com>
 <CAFjFpRc8ZoDm0+zhx+MckwGyEqkOzWcpVqbvjaxwdGarZSNrmA@mail.gmail.com>
 <5AFC0865.8050802@lab.ntt.co.jp>
 <CAFjFpRfGL9w-4C5N-ioaG3m+iheC1ey7_RMZ4vJijGn=wWJyxw@mail.gmail.com>
 <5AFD6580.5090308@lab.ntt.co.jp>
 <CAFjFpRdLCobBL7QMWPsxgm+bwWmhbaCfwOLoXXuY3DCdi96=5Q@mail.gmai
 l.com>
In-Reply-To: 
 <CAFjFpRdLCobBL7QMWPsxgm+bwWmhbaCfwOLoXXuY3DCdi96=5Q@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/17 23:19), Ashutosh Bapat wrote:
> On Thu, May 17, 2018 at 4:50 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp>  wrote:
>> (2018/05/16 22:49), Ashutosh Bapat wrote:
>>> On Wed, May 16, 2018 at 4:01 PM, Etsuro Fujita
>>> <fujita.etsuro@lab.ntt.co.jp>   wrote:
>>>> However, considering that
>>>> pull_var_clause is used in many places where partitioning is *not*
>>>> involved,
>>>> I still think it's better to avoid spending extra cycles needed only for
>>>> partitioning in that function.
>>>
>>>
>>> Right. The changes done in inheritance_planner() imply that we can
>>> encounter a ConvertRowtypeExpr even in the expressions for a relation
>>> which is not a child relation in the translated query. It was a child
>>> in the original query but after translating it becomes a parent and
>>> has ConvertRowtypeExpr somewhere there.
>>
>>
>> Sorry, I don't understand this.  Could you show me such an example?
>
> Even without inheritance we can induce a ConvertRowtypeExpr on a base
> relation which is not "other" relation
>
> create table parent (a int, b int);
> create table child () inherits(parent);
> alter table child add constraint whole_par_const check ((child::parent).a = 1);
>
> There is no way to tell by looking at the parsed query whether
> pull_var_clause() from StoreRelCheck() will encounter a
> ConvertRowtypeExpr or not. We could check whether the tables involved
> are inherited or not, but that's more expensive than
> is_converted_whole_row_reference().

Yeah, ISTM that the inheritance test makes things worse.

>>> So, there is no way to know
>>> whether a given expression will have ConvertRowtypeExpr in it. That's
>>> my understanding. But if we can device such a test, I am happy to
>>> apply that test before or witin the call to pull_var_clause().
>>>
>>> We don't need that expensive test if user has passed
>>> PVC_RECURSE_CONVERTROWTYPE. So we could test that first and then check
>>> the shape of expression tree. It would cause more asymmetry in
>>> pull_var_clause(), but we can live with it or change the order of
>>> tests for all the three options. I will differ that to a committer.
>>
>>
>> Sounds more invasive.  Considering where we are in the development cycle for
>> PG11, I think it'd be better to address this by something like the approach
>> I proposed.  Anyway, +1 for asking the committer's opinion.
>
> Thanks.

Let's ask that.

>> - On patch 0001-Handle-ConvertRowtypeExprs-in-pull_vars_clause.patch:
>>
>> +extern bool
>> +is_converted_whole_row_reference(Node *node)
>>
>> I think we should remove "extern" from the definition.
>
> Done.

>> - On patch 0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:
>>
>>
>> I think we can fix this by adding another flag to indicate whether we
>> deparsed the first live column of the relation, as in the "first" bool flag
>> in deparseTargetList.
>
> Thanks. Fixed.

Thanks for updating the patch set!  Other than pull_var_clause things, 
the updated version looks good to me, so I'll mark this as Ready for 
Committer.  As I said before, I think this issue should be addressed in 
advance of the PG11 release.

Best regards,
Etsuro Fujita