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

Hi,

On 01/25/2018 01:55 PM, David Rowley wrote:
> I've attached an updated patch which takes care of the build failure
> caused by the new call to create_append_path added in bb94ce4.
>

I've been looking at the patch over the past few days. I haven't found
any obvious issues with it, but I do have a couple of questions.

1) I can confirm that it indeed eliminates the Append overhead, using
the example from [1], modified to use table with a single partition. Of
course, this is a pretty formal check, because the patch simply removes
the Append node and so it'd be utterly broken if the overhead did not
disappear too.

[1]
https://www.postgresql.org/message-id/CAKJS1f9UXdk6ZYyqbJnjFO9a9hyHKGW7B%3DZRh-rxy9qxfPA5Gw%40mail.gmail.com

2) If I had to guess where the bugs will be, my guess would be a missing
finalize_plan_tlist call - not necessarily in existing code, but perhaps
in future improvements.

I wonder if we could automate this call somehow, say by detecting when
create_plan_recurse is called after build_path_tlist (for a given path),
and if needed calling finalize_plan_tlist from create_plan_recurse.

Although, now that I look at it, promote_child_relation may be doing
something like that by adding stuff to the translate_* variables. I'm
not quite sure why we need to append to the lists, though?

3) I'm not quite sure what replace_translatable_exprs does, or more
precisely why do we actually need it. At this point the comment pretty
much says "We need to do translation. This function does translation."
Perhaps it should mention why removing/skipping the AppendPath implies
the need for translation or something?

4) The thread talks about "[Merge]Append" but the patch apparently only
tweaks create_append_path and does absolutely nothing to "merge" case.
While looking into why, I notices that generate_mergeappend_paths always
gets all_child_pathkeys=NULL in this case (with single subpath), and so
we never create the MergePath at all. I suppose there's some condition
somewhere responsible for doing that, but I haven't found it ...

5) The comment before AppendPath typedef says this:

* An AppendPath with a single subpath can be set up to become a "proxy"
* path. This allows a Path which belongs to one relation to be added to
* the pathlist of some other relation. This is intended as generic
* infrastructure, but its primary use case is to allow Appends with
* only a single subpath to be removed from the final plan.

I'm not quite sure how adding a 'isproxy' flag into AppendPath node
makes this a generic infrastructure? Wouldn't an explicit ProxyPath be a
better way to achieve that?

What other proxy paths do you envision, and why should they be
represented as AppendPath? Although, there seems to be a precedent in
using AppendPath to represent dummy paths ...

FWIW one advantage of a separate ProxyPath would be that it would be a
much clearer breakage for third-party code (say, extensions tweaking
paths using hooks), either at compile or execution time. With just a new
flag in existing node, that may easily slip through. On the other hand
no one promises the structures to be stable API ...

6) I suggest we add this assert to create_append_path:

Assert(!isproxy || (isproxy && (list_length(subpaths)==1)));

and perhaps we should do s/isproxy/is_proxy/ which seems like the usual
naming for boolean variables.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services