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

(2018/07/21 0:26), Robert Haas wrote:
> On Fri, Jul 20, 2018 at 8:38 AM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> This looks like a terrible design to me. If somebody in future
>> invents a new plan type that is not projection-capable, then this
>> could fail an assertion here and there won't be any simple fix. And
>> if you reach here and the child targetlists somehow haven't been
>> adjusted, then you won't even fail an assertion, you'll just crash (or
>> maybe return wrong answers).
>
> To elaborate on this thought a bit, it is currently the case that all
> scan and join types are projection-capable, and most likely we would
> make any such node types added in the future projection-capable as
> well, but it still doesn't seem like a good idea to me to have
> tangentially-related parts of the system depend on that in subtle
> ways.

I have to admit that that is not a good idea. So, I'll update the patch
so that we don't assume the projection capability of the subplan
anymore, if we go this way.

> Also, even today, this would fail if the subplan happened to be
> a Sort, and it's not very obvious why that couldn't happen.

You mean the MergeAppend case? In that case, the patch will adjust the
subplan before adding a Sort above the subplan, so that could not happen.

> More broadly, the central idea of this patch is that we can set the
> target list for a child rel to something other than the target list
> that we expect it will eventually produce, and then at plan creation
> time fix it.

Yeah, the patch would fix the the final output to the appendrel parent
at plan creation time if necessary, but it would produces the tlist of
an intermediate child scan/join node as written in the child relation's
reltarget at that time.

> I think that's a bad idea. The target list affects lots
> of things, like costing. If we don't insert a ConvertRowTypeExpr into
> the child's target list, the costing will be wrong to the extent that
> ConvertRowTypeExpr has any cost, which it does.

Actually, this is not true at least currently, because
set_append_rel_size doesn't do anything about the costing:

* NB: the resulting childrel->reltarget->exprs may contain
arbitrary
* expressions, which otherwise would not occur in a rel's
targetlist.
* Code that might be looking at an appendrel child must cope with
* such. (Normally, a rel's targetlist would only include Vars and
* PlaceHolderVars.) XXX we do not bother to update the cost
or width
* fields of childrel->reltarget; not clear if that would be
useful.

> Any other current or
> future property that is computed based on the target list --
> parallel-safety, width, security-level, whatever -- could also
> potentially end up with a wrong value if the target list as written
> does not match the target list as will actually be produced. It's
> true that, in all of these areas, the ConvertRowTypeExpr isn't likely
> to have a lot of impact; it probably won't have a big effect on
> costing and may not actually cause a problem for the other things that
> I mentioned. On the other hand, if it does cause a serious problem,
> what options would we have for fixing it other than to back out your
> entire patch and solve the problem some other way? The idea that
> derived properties won't be correct unless they are based on the real
> target list is pretty fundamental, and I think deviating from the idea
> that we get the correct target list first and then compute the derived
> properties afterward is likely to cause real headaches for future
> developers.
>
> 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.

Some createplan.c routines already change the tlists of their nodes.
For example, create_merge_append_plan would add sort columns to each of
its subplans if necessary. I think it would be similar to that to add a
ConvertRowtypeExpr above a child whole-row Var in the subplan's tlist.

> 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 reviewed his patch, and thought that that would fix the issue, but
this is about the current design on the child tlist handling in
partitionise join rather than that patch: it deviates from the
assumption that we had for the scan/join planning till PG10 that "a
rel's targetlist would only include Vars and PlaceHolderVars", and turns
out that there are a lot of places where we need to modify code to
suppress that deviation, as partly shown in that patch. So, ISTM that
the current design in itself is not that localized.

> 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 on that point.

Best regards,
Etsuro Fujita