(2018/07/21 0:26), Robert Haas wrote:
On Fri, Jul 20, 2018 at 8:38 AM, Robert Haas<robertmh...@gmail.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
