MIME-Version: 1.0
In-Reply-To: <5B17C5EC.1090602@lab.ntt.co.jp>
References: 
 <CAKcux6ktu-8tefLWtQuuZBYFaZA83vUzuRd7c1YHC-yEWyYFpg@mail.gmail.com>
 <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> <5AFE81CC.8000508@lab.ntt.co.jp>
 <5B17C5EC.1090602@lab.ntt.co.jp>
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Date: Thu, 7 Jun 2018 16:12:58 +0530
Message-ID: 
 <CAFjFpRffA-BfL0RXcUVLLbcOBF1ie7oObTB-RE2q_o45X1HTYg@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 Wed, Jun 6, 2018 at 5:00 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> (2018/05/18 16:33), Etsuro Fujita wrote:
>>
>> Other than pull_var_clause things,
>> the updated version looks good to me, so I'll mark this as Ready for
>> Committer.
>
>
> 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.

>  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.

Given these serious problems, I don't think we could afford not to
cover a child whole-row reference in ConvertRowtypeExpr.

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