(2018/08/02 4:30), Robert Haas wrote:
On Wed, Aug 1, 2018 at 7:44 AM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
I updated the patch that way. Updated patch attached. I fixed a bug and
did a bit of cleanups as well.
Looking this over from a technical point of view, I think it's better
than what you proposed before but I still don't agree with the
approach. I don't think there is any getting around the fact that
converted whole-row vars are going to result in some warts. Ashutosh
inserted most of the necessary warts in the original partition-wise
join patch, but missed some cases in postgres_fdw; his approach is,
basically, to add the matching warts there. Your approach is to rip
out all of those warts and insert different ones. You've simplified
build_tlist_index_other_vars() and add_placeholders_to_joinrel() and
build_joinrel_tlist() to basically what they were before
partition-wise join went in. On the other hand, RelOptInfo grows
three new fields,
Sorry, I forgot to remove one of the fields that isn't needed anymore.
RelOptInfo needs two new fields, in the new approach.
set_append_rel_size() ends up more complicated than
it was before when you include the node code added in the
build_childrel_tlist function it calls, build_child_joinrel() has a
different set of complications, and most importantly
create_append_path() and create_merge_append_path() now do surgery on
their input paths that knows specifically about the
converted-whole-row var case. I would be glad to be rid of the stuff
that you're ripping out, but in terms of complexity, I don't think we
really come out ahead with the stuff you're adding.
The new approach seems to me more localized than what Ashutosh had
proposed. One obvious advantage of the new approach is that we no
longer need changes to postgres_fdw for it to work for partitionwise
joins, which would apply for any other FDWs, making the FDW authors'
life easy.
I'm also still concerned about the design. In your old approach, you
were creating the paths with with the wrong target list and then
fixing it when you turned the paths into a plan. In your new
approach, you're still creating the paths with the wrong target list,
but now you're fixing it when you put an Append or MergeAppend path on
top of them. I still think it's a bad idea to have anything other
than the correct paths in the target list from the beginning.
I don't think so; actually, the child's targetlist would not have to be
the same as the parent's until the child gets in under the parent's
Append or MergeAppend. So, I modified the child's target list so as to
make it easy to join the child relations.
For one
thing, what happens if no Append or MergeAppend is ever put on top of
them? One way that can happen today is partition-wise aggregate,
although I haven't been able to demonstrate a bug, so maybe that's
covered somehow.
Right. In the new approach, the targetlist for the topmost child
scan/join is guaranteed to be the same as that for the topmost parent
scan/join by the planner work that I added.
But in general, with your approach, any code that
looks at the tlist for a child-join has to know that the tlist is to
be used as-is *except that* ConvertRowtypeExpr may need to be
inserted. I think that special case could be easy to overlook, and it
seems pretty arbitrary.
I think we can check to see whether the conversion is needed, from the
flags added to RelOptInfo.
A tlist is a list of arbitrary expressions to
be produced; with this patch, we'd end up with the tlist being the
list of expressions to be produced in every case *except for*
child-joins containing whole-row-vars. I just can't get behind a
strange exception like that.
I have to admit that it's weird to adjust the child's targetlist in that
case when putting the child under the parent's Append/MergeAppend, but
IMHO I think that would be much localized.
Thanks for the comments!
Best regards,
Etsuro Fujita
