Message-ID: <5B1E5071.1000704@lab.ntt.co.jp>
Date: Mon, 11 Jun 2018 19:35:29 +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>
 <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> <5AFE81CC.8000508@lab.ntt.co.jp>
 <5B17C5EC.1090602@lab.ntt.co.jp>
 <CAFjFpRffA-BfL0RXcUVLLbcOBF1ie7oObTB-RE2q_o45X1HTYg@mail.gmai
 l.com>
In-Reply-To: 
 <CAFjFpRffA-BfL0RXcUVLLbcOBF1ie7oObTB-RE2q_o45X1HTYg@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/06/07 19:42), Ashutosh Bapat wrote:
> On Wed, Jun 6, 2018 at 5:00 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp>  wrote:
>> Since I'm not 100% sure that that is the right way to go, I've been
>> rethinking how to fix this issue.  Yet another idea I came up with to fix
>> this is to redesign the handling of the tlists for children in the
>> partitioning case.  Currently, we build the reltarget for a child by
>> applying adjust_appendrel_attrs to the reltarget for its parent in
>> set_append_rel_size, which maps a wholerow Var referring to the parent rel
>> to a ConvertRowtypeExpr that translates a wholerow of the child rel into a
>> wholerow of the parent rel's rowtype.  This works well for the
>> non-partitioned inheritance case, but makes complicated the code for
>> handling the partitioning case especially when planning partitionwise-joins.
>> And I think this would be the root cause of this issue.
>
> Although true, this is not only about targetlist. Even the whole-row
> expressions in the conditions, equivalence classes and other
> planner/optimizer data structures are translated to
> ConvertRowtypeExpr.

Yeah, but I mean we modify set_append_rel_size so that we only map a 
parent wholerow Var in the parent tlist to a child wholerow Var in the 
child's tlist; parent wholerow Vars in the parent's other expressions 
such as conditions are transformed into CREs as-is.

>>   I don't think the
>> tlists for the children need to have their wholerows transformed into the
>> corresponding ConvertRowtypeExprs *at this point*, so what I'd like to
>> propose is to 1) map a parent wholerow Var simply to a child wholerow Var,
>> instead (otherwise, the same as adjust_appendrel_attrs), when building the
>> reltarget for a child in set_append_rel_size, 2) build the tlists for child
>> joins using those children's wholerow Vars at path creation time, and 3)
>> adjust those wholerow Vars in the tlists for subpaths in the chosen
>> AppendPath so that those are transformed into the corresponding
>> ConvertRowtypeExprs, at plan creation time (ie, in
>> create_append_plan/create_merge_append_plan).  IIUC, this would not require
>> any changes to pull_var_clause as proposed by the patch.  This would not
>> require any changes to postgres_fdw as proposed by the patch, either.  In
>> addition, this would make unnecessary the code added to setrefs.c by the
>> partitionwise-join patch.  Maybe I'm missing something though.
>
> Not translating whole-row expressions to ConvertRowtypeExpr before
> creating paths can lead to a number of anomalies. For example,
>
> 1. if there's an index on the whole-row expression of child,
> translating parent's whole-row expression to child's whole-row
> expression as is will lead to using that index, when in reality the it
> can not be used since the condition/ORDER BY clause (which originally
> referred the parent's whole-row expression) require the child's
> whole-row reassembled as parent's whole-row.
> 2. Constraints on child'd whole-row expression, will be used to prune
> a child when they can not be used since the original condition was on
> parent' whole-row expression and not that of the child.
> 3. Equivalence classes will think that a child whole-row expression
> (without ConvertRowtypeExpr) is equivalent to an expression which is
> part of the same equivalence class as the parent' whole-row
> expression.

Is that still possible when we only map a parent wholerow Var in the 
parent's tlist to a child wholerow Var in the child's tlist?

Best regards,
Etsuro Fujita