On Fri, Jul 20, 2018 at 8:56 PM, Robert Haas <robertmh...@gmail.com> wrote: [ ... clipped portion ...] > > In short, plan creation time is just the wrong time to change the > plan. It's just a time to translate the plan from the form needed by > the planner to the form needed by the executor. It would be fine if > the change you were making were only cosmetic, but it's not. >
Agree with all that, including the clipped paras. > After looking over both patches, I think Ashutosh Bapat has basically > the right idea. What he is proposing is a localized fix that doesn't > seem to make any changes to the way things work overall. I do think > his patches need to be fixed up a bit to avoid conflating > ConvertRowtypeExpr with "converted whole-row reference." The two are > certainly not equivalent; the latter is a narrow special case. Some > of the comments could use some more wordsmithing, too, I think. Apart > from those gripes, though, I think it's solving the problem the > correct way: don't build the wrong plan and try to fix it, just build > the right plan from the beginning. I will work on that if everyone agrees that that's the right way to go. Fujita-san seems to have some arguments still. I have argued on the same lines as yours but your explanation is better. I don't have anything more to add. I will wait for that to be resolved. > > There are definitely some things not to like about this approach. In > particular, I definitely agree that treating a converted whole-row > reference specially is not very nice. It feels like it might be > substantially cleaner to be able to represent this idea as a single > node rather than a combination of ConvertRowtypeExpr and var with > varattno = 0. Perhaps in the future we ought to consider either (a) > trying to use a RowExpr for this rather than ConvertRowtypeExpr or (b) > inventing a new WholeRowExpr node that stores two RTIs, one for the > relation that's generating the whole-row reference and the other for > the relation that is controlling the column ordering of the result or > (c) allowing a Var to represent a whole-row expression that has to > produce its outputs according to the ordering of some other RTE. I never liked representing a whole-row expression as a Var worst with varattno = 0. We should have always casted it as RowExpr(set of vars, one var per column). This problem wouldn't arise then. Many other problems wouldn't arise then, I think. I think we do that so that we can convert a tuple from buffer into a datum and put it in place of Var with varattno = 0. Given that I prefer (a) or (b) in all the cases. If not that, then c. But I agree that we have to avoid ConvertRowtypeExpr being used in this case. > But > I don't think it's wise or necessary to whack that around just to fix > this bug; it is refactoring or improvement work best left to a future > release. > > Also, it is definitely a real shame that we have to build attr_needed > data for each child separately. Right now, we've got to build a whole > lot of things for each child individually, and that's slow and > inefficient. We're not going to scale nicely to large partitioning > hierarchies unless we can figure out some way of minimizing the work > we do for each partition. So, the fact that Fujii-san's approach > avoids needing to compute attr_needed in some cases is very appealing. > However, in my opinion, it's nowhere near appealing enough to justify > trying to do surgery on the target list at plan-creation time. I > think we need to leave optimizing this to a future effort. > Partition-wise join/aggregate, and partitioning in general, need a lot > of optimization in a lot of places, and fortunately there are people > working on that, but our goal here should just be to fix things up > well enough that we can ship it. I agree. > I don't see anything in Ashutosh's > patch that is so ugly that we can't live with it for the time being. Thanks. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
