MIME-Version: 1.0
In-Reply-To: <5B619D23.4010000@lab.ntt.co.jp>
References: <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.gmail.com>
 <5B1E5071.1000704@lab.ntt.co.jp> <5B23A964.9020800@lab.ntt.co.jp>
 <5B28FB19.3090809@lab.ntt.co.jp>
 <CA+TgmoZboTszGQk3OecBOZUFNNjzKE-SOo_Q46xtTJ4Yj95-RQ@mail.gmail.com>
 <20180703163519.tsigyic5xefunv74@alap3.anarazel.de>
 <5B3C2A4E.5030901@lab.ntt.co.jp>
 <5B4F2BE9.3000304@lab.ntt.co.jp>
 <CA+TgmobR9XWEhmF3s_WsoLNnnjaFyS_xtbrVG3dCGiUgof20Nw@mail.gmail.com>
 <CA+TgmoZSaKq-fYALn5jf6c_X3==Rb2s8eqLDwGpV=LNNhTXYwg@mail.gmail.com>
 <5B55886F.5090906@lab.ntt.co.jp>
 <CA+TgmoYYTCQL=TUYbUz2b=CsQeB4o1L2+_=zHynxGVmyKUtH1A@mail.gmail.com>
 <5B5712C8.9030404@lab.ntt.co.jp>
 <CA+TgmoazFzdp5ZVwUjk_b2cuB90WCeeWtzt1ShSbXGyg67c+Zw@mail.gmail.com>
 <5B59BA55.30200@lab.ntt.co.jp> <5B619D23.4010000@lab.ntt.co.jp>
From: Robert Haas <robertmhaas@gmail.com>
Date: Wed, 1 Aug 2018 15:30:07 -0400
Message-ID: 
 <CA+TgmoZMdNiKUGV0=-QLV1f1-Xr0CDT+8y9wLktuEiQGkt64UQ@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: Andres Freund <andres@anarazel.de>,
 Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>,
	Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>, Tom Lane <tgl@sss.pgh.pa.us>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Aug 1, 2018 at 7:44 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> I updated the patch that way.  Updated patch attached.  I fixed a bug and
> did a bit of cleanups as well.

Looking this over from a technical point of view, I think it's better
than what you proposed before but I still don't agree with the
approach.  I don't think there is any getting around the fact that
converted whole-row vars are going to result in some warts.  Ashutosh
inserted most of the necessary warts in the original partition-wise
join patch, but missed some cases in postgres_fdw; his approach is,
basically, to add the matching warts there.  Your approach is to rip
out all of those warts and insert different ones.  You've simplified
build_tlist_index_other_vars() and add_placeholders_to_joinrel() and
build_joinrel_tlist() to basically what they were before
partition-wise join went in.  On the other hand, RelOptInfo grows
three new fields, set_append_rel_size() ends up more complicated than
it was before when you include the node code added in the
build_childrel_tlist function it calls, build_child_joinrel() has a
different set of complications, and most importantly
create_append_path() and create_merge_append_path() now do surgery on
their input paths that knows specifically about the
converted-whole-row var case.  I would be glad to be rid of the stuff
that you're ripping out, but in terms of complexity, I don't think we
really come out ahead with the stuff you're adding.

I'm also still concerned about the design.  In your old approach, you
were creating the paths with with the wrong target list and then
fixing it when you turned the paths into a plan.  In your new
approach, you're still creating the paths with the wrong target list,
but now you're fixing it when you put an Append or MergeAppend path on
top of them.  I still think it's a bad idea to have anything other
than the correct paths in the target list from the beginning.  For one
thing, what happens if no Append or MergeAppend is ever put on top of
them?  One way that can happen today is partition-wise aggregate,
although I haven't been able to demonstrate a bug, so maybe that's
covered somehow.  But in general, with your approach, any code that
looks at the tlist for a child-join has to know that the tlist is to
be used as-is *except that* ConvertRowtypeExpr may need to be
inserted.  I think that special case could be easy to overlook, and it
seems pretty arbitrary.  A tlist is a list of arbitrary expressions to
be produced; with this patch, we'd end up with the tlist being the
list of expressions to be produced in every case *except for*
child-joins containing whole-row-vars.  I just can't get behind a
strange exception like that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company