MIME-Version: 1.0
In-Reply-To: <13665.1533306968@sss.pgh.pa.us>
References: <5AFC0865.8050802@lab.ntt.co.jp> <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>
 <CA+TgmoZMdNiKUGV0=-QLV1f1-Xr0CDT+8y9wLktuEiQGkt64UQ@mail.gmail.com>
 <CA+TgmoYJ_HmMjGr-y2fg71XQ6J3S9ro6oehJmYkkoGVwZSwe7w@mail.gmail.com>
 <12308.1533305221@sss.pgh.pa.us>
 <CA+TgmoZmVEx3piQ5TSKMyv55Rt8osEFeEUY1497SBXqnYMuu7g@mail.gmail.com>
 <13665.1533306968@sss.pgh.pa.us>
From: Robert Haas <robertmhaas@gmail.com>
Date: Fri, 3 Aug 2018 13:23:29 -0400
Message-ID: 
 <CA+TgmoYPaDeM9n8E+mO=d3CRxnJNHfxW7w77SG_YjvWHgqgVig@mail.gmail.com>
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with
 partition wise join enabled.
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>,
 Andres Freund <andres@anarazel.de>,
	Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>,
	Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Aug 3, 2018 at 10:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Anyway, what I'm basically suggesting is that we just disable support for
> PWJ in the problematic cases in v11.  As long as PWJ isn't even on by
> default, that's not much of a loss.  Obviously we'll want to fix it in the
> future, but the hour grows late for v11, and I think either of these
> patches would need quite a bit more work to be committable.

That's not my first choice, but I'm OK with it.

>> There are definitely some things not to like about this approach.  In
>> particular, I definitely agree that treating a converted whole-row
>> reference specially is not very nice.  It feels like it might be
>> substantially cleaner to be able to represent this idea as a single
>> node rather than a combination of ConvertRowtypeExpr and var with
>> varattno = 0.  Perhaps in the future we ought to consider either (a)
>> trying to use a RowExpr for this rather than ConvertRowtypeExpr or (b)
>> inventing a new WholeRowExpr node that stores two RTIs, one for the
>> relation that's generating the whole-row reference and the other for
>> the relation that is controlling the column ordering of the result or
>> (c) allowing a Var to represent a whole-row expression that has to
>> produce its outputs according to the ordering of some other RTE.  But
>> I don't think it's wise or necessary to whack that around just to fix
>> this bug; it is refactoring or improvement work best left to a future
>> release.
>
> I agree with all of that except your conclusion that it's unnecessary
> to do it to fix this bug.  I find that entirely unsupported and over-
> optimistic, especially in view of the number of iterations Ashutosh
> went through trying to fix the fallout from not making a clear
> distinction.

I don't think that the question of how a converted whole-row expr can
really make a difference between being able to fix this bug and not
being able to fix this bug.  It's just a representational choice.  If
we decide that a converted whole-row expr is represented as
ConvertRowTypeExpr on top of a Var, then we have to check for that.
If we revise the representation to use a Var node directly, or to use
a new WholeRowVar node, then we'll just need to check for those things
instead.  I think that the shape of the fix itself does not change.
Moreover, it's pretty much a 1:1 mapping.  It's not like, say,
replacing plans with paths in the whole upper half of the planner,
where the representation is different enough that things that would
have been very difficult to manage in the old representation become
relatively simple in the new one.  It's basically just a question of
how you spell it.  Writing a test for ConvertRowTypeExpr atop Var and
using it a bunch of places is not beautiful, and it's certainly going
to be marginally slower than a direct test for WholeRowVar or some
other representation that bundles the whole thing into one node, but I
think it should work just fine, and the representation can be revised
later in a separate patch once we agree on an approach.

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