On Thu, 3 Jan 2019 at 08:01, Tomas Vondra <tomas.von...@2ndquadrant.com> wrote: > AFAICS the patch essentially does two things: (a) identifies Append > paths with a single member and (b) ensures the Vars are properly mapped > when the Append node is skipped when creating the plan.
Yes, but traditional Append and MergeAppend paths with a single member are never actually generated. I say "traditional" as we do happen to use a single-subpath Append as the "proxy" path to represent a path that needs to not have a plan node generated during createplan. Originally I had a new Path type named "ProxyPath" to do this, but per recommendation from Tom, I overloaded Append to mean that. (Append already has its meaning overloaded in regards to dummy paths.) > I agree doing (a) in setrefs.c is too late, as mentioned earlier. But > perhaps we could do at least (b) to setrefs.c? The problem I found with doing var translations in setrefs.c was that the plan tree is traversed there breadth-first and in createplan.c we build the plan depth-first. The problem I didn't manage to overcome with that is that when we replace a "ProxyPath" (actually an Append path marked as is_proxy=true) it only alter targetlists, etc of nodes above that in the plan tree. The nodes below it and their targetlists don't care, as these don't fetch Vars from nodes above them. If we do the Var translation in setrefs then we've not yet looked at the nodes that don't care and we've already done the nodes that do care. So the tree traversal is backwards. I sort of borrowed the traversal in createplan.c since it was in the correct depth-first order. Equally, I could have invented an entirely new stage that traverses the path tree depth-first, but I imagined that would have added more overhead and raised even more questions. Piggy-backing on createplan seemed like the best option at the time. > I'd say mapping the Vars is the most fragile and error-prone piece of > this patch. It's a bit annoying that it's inventing a new infrastructure > to do that, translates expressions in various places, establishes new > rules for tlists (having to call finalize_plan_tlist when calling > build_path_tlist before create_plan_recurse), etc. I entirely agree. It's by far the worst part of the patch. Getting the code to a working state to do this was hard. I kept finding places that I'd missed the translation. I'd rather it worked some other way. I just don't yet know what that way is. I changed the core regression table "tenk" to become a partitioned table with a single partition in the hope to catch all of the places that need the translations to be performed. I'm not entirely confident that I've caught them all by doing this. I've attached an updated patch that's really just a rebased version of v8. The old version conflicted with changes made in 1db5667b. The only real change was to the commit message. I'd previously managed to miss out the part about not generating needless Append/MergeAppend paths can allow plan shapes that were previously not possible. I'd only mentioned the executor not having to pull tuples through these nodes, which increases performance. Not having this perhaps caused some of the confusion earlier in this thread. One drawback to this patch, which I'm unsure if I previously mentioned is that run-time pruning only works for Append and MergeAppend. If we remove the Append/MergeAppend node then the single Append/MergeAppend sub-plan has no hope of being pruned. Going from 1 to 0 sub-plans may not be that common, but no longer supporting that is something that needs to be considered. I had imagined that gained flexibility of plan shapes plus less executor overhead was better than that, but there likely is a small use-case for keeping that ability. It seems impossible to have both advantages of this patch without that disadvantage. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
v9-0001-Forgo-generating-single-subpath-Append-and-MergeA.patch
Description: Binary data