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
