MIME-Version: 1.0
In-Reply-To: 
 <CA+TgmoZSaKq-fYALn5jf6c_X3==Rb2s8eqLDwGpV=LNNhTXYwg@mail.gmail.com>
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>
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Date: Mon, 23 Jul 2018 15:08:41 +0530
Message-ID: 
 <CAFjFpRdqSt9hCA5JbkZdYN+e3NDAOHzFCDp0xqY92+Mcyi_M3g@mail.gmail.com>
Subject: Re: Expression errors with "FOR UPDATE" and postgres_fdw with
 partition wise join enabled.
To: Robert Haas <robertmhaas@gmail.com>
Cc: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>,
 Andres Freund <andres@anarazel.de>,
	Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jul 20, 2018 at 8:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:
[ ... clipped portion ...]
>
> In short, plan creation time is just the wrong time to change the
> plan.  It's just a time to translate the plan from the form needed by
> the planner to the form needed by the executor.  It would be fine if
> the change you were making were only cosmetic, but it's not.
>

Agree with all that, including the clipped paras.

> After looking over both patches, I think Ashutosh Bapat has basically
> the right idea.  What he is proposing is a localized fix that doesn't
> seem to make any changes to the way things work overall.  I do think
> his patches need to be fixed up a bit to avoid conflating
> ConvertRowtypeExpr with "converted whole-row reference."  The two are
> certainly not equivalent; the latter is a narrow special case.  Some
> of the comments could use some more wordsmithing, too, I think.  Apart
> from those gripes, though, I think it's solving the problem the
> correct way: don't build the wrong plan and try to fix it, just build
> the right plan from the beginning.

I will work on that if everyone agrees that that's the right way to
go. Fujita-san seems to have some arguments still. I have argued on
the same lines as yours but your explanation is better. I don't have
anything more to add. I will wait for that to be resolved.

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

I never liked representing a whole-row expression as a Var worst with
varattno = 0. We should have always casted it as RowExpr(set of vars,
one var per column). This problem wouldn't arise then. Many other
problems wouldn't arise then, I think. I think we do that so that we
can convert a tuple from buffer into a datum and put it in place of
Var with varattno = 0. Given that I prefer (a) or (b) in all the
cases. If not that, then c. But I agree that we have to avoid
ConvertRowtypeExpr being used in this case.

> 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.
>
> Also, it is definitely a real shame that we have to build attr_needed
> data for each child separately.  Right now, we've got to build a whole
> lot of things for each child individually, and that's slow and
> inefficient.  We're not going to scale nicely to large partitioning
> hierarchies unless we can figure out some way of minimizing the work
> we do for each partition.  So, the fact that Fujii-san's approach
> avoids needing to compute attr_needed in some cases is very appealing.
> However, in my opinion, it's nowhere near appealing enough to justify
> trying to do surgery on the target list at plan-creation time.  I
> think we need to leave optimizing this to a future effort.
> Partition-wise join/aggregate, and partitioning in general, need a lot
> of optimization in a lot of places, and fortunately there are people
> working on that, but our goal here should just be to fix things up
> well enough that we can ship it.

I agree.

> I don't see anything in Ashutosh's
> patch that is so ugly that we can't live with it for the time being.

Thanks.

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