MIME-Version: 1.0
In-Reply-To: <5B46CBB3.2090300@lab.ntt.co.jp>
References: 
 <CAKcux6ktu-8tefLWtQuuZBYFaZA83vUzuRd7c1YHC-yEWyYFpg@mail.gmail.com>
 <5B1E5071.1000704@lab.ntt.co.jp> <5B23A964.9020800@lab.ntt.co.jp>
 <5B28FB19.3090809@lab.ntt.co.jp>
 <CAFjFpRedQBySj5Vm-eq+gATP6sqJJfXzPrV+Ld+K6DnaOGkFAQ@mail.gmail.com>
 <5B362B38.2000103@lab.ntt.co.jp>
 <CAFjFpRf55MT-9NnaRwp0wkDUTuwb++UA_un9Twai=qNVePh-TQ@mail.gmail.com>
 <5B3CB833.4040005@lab.ntt.co.jp>
 <CAFjFpRevODMmZ8MF3oLsXr9mSH9-mWC_fxmPWTYCSqHVr_Zwqg@mail.gmail.com>
 <5B3F4BAA.3050601@lab.ntt.co.jp>
 <CAFjFpRf2vd0C6B8g1TmpJN8RkPECgZy8iGe9UYZTy9EWu1eL2Q@mail.gmail.com>
 <5B4340E9.3030707@lab.ntt.co.jp>
 <CAFjFpRcwrESJBDkmm1kvtV234nkw=dr9nJ=RWJ4pmsVQqF4avg@mail.gmail.com>
 <5B434886.9010107@lab.ntt.co.jp>
 <CAFjFpRf-t_dM=qirB5+95wT1hSCLs8mQqbfRSKP=xUuhnOhbOQ@mail.gmail.com>
 <5B442A40.2040903@lab.ntt.co.jp>
 <CAFjFpRfxsg3dXEDQPdUD+HYqE6v=jEYFiEtEgXuF+q+MVpA8eg@mail.gmail.com>
 <5B45B777.7010502@lab.ntt.co.jp>
 <CAFjFpRf-n75_mN=EFAvY-YY1xd_AXEDUdedW7sDX5poxpqs7fA@mail.gmail.com>
 <5B46CBB3.2090300@lab.ntt.co.jp>
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Date: Thu, 12 Jul 2018 10:08:13 +0530
Message-ID: 
 <CAFjFpReZqRx4FE1EZBBm4FGBem8u-8ijoPT=xcXsyfz5eTqpiw@mail.gmail.com>
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with
 partition wise join enabled.
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
Cc: Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jul 12, 2018 at 9:02 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> (2018/07/11 20:02), Ashutosh Bapat wrote:
>>
>> On Wed, Jul 11, 2018 at 1:23 PM, Etsuro Fujita
>> <fujita.etsuro@lab.ntt.co.jp>  wrote:
>>>
>>> Actually, even if we could create such an index on the child table and
>>> the
>>> targetlist had the ConvertRowtypeExpr, the planner would still not be
>>> able
>>> to use an index-only scan with that index; because check_index_only would
>>> not consider that an index-only scan is possible for that index because
>>> that
>>> index is an expression index and that function currently does not
>>> consider
>>> that index expressions are able to be returned back in an index-only
>>> scan.
>>> That behavior of the planner might be improved in future, though.
>
>
>> Right and when we do so, not having ConvertRowtypeExpr in the
>> targetlist will be a problem.
>
>
> Yeah, but I don't think that that's unsolvable; because in that case the CRE
> as an index expression could be converted back to the whole-row Var's
> rowtype by adding another CRE to the index expression for that conversion, I
> suspect that that special handling could allow us to support an index-only
> scan even when having the whole-row Var instead of the CRE in the
> targetlist.

I am not able to understand this. Can you please provide an example?

> (Having said that, I'm not 100% sure we need to solve that
> problem when we improve the planner, because there doesn't seem to me to be
> enough use-case to justify making the code complicated for that.)  Anyway, I
> think that that would be a matter of future versions of PG.

I am afraid that a fix which just works only till we change the other
parts of the system is useful only till that time. At that time, it
needs to be replaced with a different fix. If that time is long
enough, that's ok. In this case, I agree that if we haven't heard from
users till now that they need to create indexes on whole-row
expressions, there's chance that we will never implement this feature.

>
>>>> At places in planner we match equivalence members
>>>> to the targetlist entries. This matching will fail unexpectedly when
>>>> ConvertRowtypeExpr is removed from a child's targetlist. But again I
>>>> couldn't reproduce a problem when such a mismatch arises.
>>>
>>>
>>>
>>> IIUC, I don't think the planner assumes that for an equivalence member
>>> there
>>> is an matching entry for that member in the targetlist; what I think the
>>> planner assumes is: an equivalence member is able to be computed from
>>> expressions in the targetlist.
>>
>>
>> This is true. However,
>>
>>>   So, I think it is safe to have whole-row
>>> Vars instead of ConvertRowtypeExprs in the targetlist.
>>
>>
>> when it's looking for an expression, it finds a whole-row expression
>> so it think it needs to add a ConvertRowtypeExpr on that. But when the
>> plan is created, there is ConvertRowtypeExpr already, but there is no
>> way to know that a new ConvertRowtypeExpr is not needed anymore. So,
>> we may have two ConvertRowtypeExprs giving wrong results.
>
>
> Maybe I'm missing something, but I don't think that we need to worry about
> that, because in the approach I proposed, we only add CREs above whole-row
> Vars in the targetlists for subplans of an Append/MergeAppend for a
> partitioned relation at plan creation time.

There's a patch in an adjacent thread started by David Rowley to rip
out Append/MergeAppend when there is only one subplan. So, your
solution won't work there.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company