Re: [HACKERS] Removing [Merge]Append nodes which contain a single subpath

On Mon, 4 Mar 2019 at 16:26, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> > [ v13-0001-Forgo-generating-single-subpath-Append-and-Merge.patch ]
>
> I continue to think that this is the wrong way to go about it,
> and as proof of concept present the attached, which reproduces
> all of the regression-test plan changes appearing in v13 ---
> with a whole lot less mechanism and next to no added planning
> cycles (which certainly cannot be said of v13).

Nice. I see you solved the problem I had been complaining about by
pulling the surplus Append out of the final plan after setrefs, in
which case the varnos are all set to the special varnos, meaning you
don't suffer from the same problem with nodes higher in the plan
having the varnos of the parent instead of the append child. This
method is certainly much much better than what I had. Thanks for
taking the time to show me how this should be done.

> Also, I wonder why you didn't teach ExecSupportsMarkRestore that a
> single-child MergeAppend can support mark/restore. I didn't add such
> code here either, but I suspect that's an oversight.

The reason for that was that I didn't ever create any single-subpath
MergeAppends. I instead created Append paths having isproxy as true.
This is slightly confusing as I was just borrowing Append to act as
this proxy path and it was only these proxy Appends I was dealing with
in ExecSupportsMarkRestore.

I'll need to modify your version of the patch as you're keeping
single-subpath MergeAppends paths, so ExecSupportsMarkRestore needs to
know about those.

> One other remark is that the division of labor between
> create_[merge]append_path and their costsize.c subroutines
> seems pretty unprincipled. I'd be inclined to push all the
> relevant logic into costsize.c, but have not done so here.
> Moving the existing cost calculations in create_mergeappend_path
> into costsize.c would better be done as a separate refactoring patch,
> perhaps.

The part I don't like about that is the fact that we don't generate
sort paths in the MergeAppend subpaths, instead, we rely on checking
pathkeys_contained_in() again in create_merge_append_plan() and just
creating a sort node, if required. There is some repeat pathkeys
checking there but I wonder if we'll see much a performance regression
if we go to the trouble of making sort paths. Is this what you meant?

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services