Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

On Fri, Jul 20, 2018 at 8:56 PM, Robert Haas <robertmhaas(at)gmail(dot)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