On Wed, Jun 6, 2018 at 5:00 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > (2018/05/18 16:33), Etsuro Fujita wrote: >> >> Other than pull_var_clause things, >> the updated version looks good to me, so I'll mark this as Ready for >> Committer. > > > Since I'm not 100% sure that that is the right way to go, I've been > rethinking how to fix this issue. Yet another idea I came up with to fix > this is to redesign the handling of the tlists for children in the > partitioning case. Currently, we build the reltarget for a child by > applying adjust_appendrel_attrs to the reltarget for its parent in > set_append_rel_size, which maps a wholerow Var referring to the parent rel > to a ConvertRowtypeExpr that translates a wholerow of the child rel into a > wholerow of the parent rel's rowtype. This works well for the > non-partitioned inheritance case, but makes complicated the code for > handling the partitioning case especially when planning partitionwise-joins. > And I think this would be the root cause of this issue.
Although true, this is not only about targetlist. Even the whole-row expressions in the conditions, equivalence classes and other planner/optimizer data structures are translated to ConvertRowtypeExpr. > I don't think the > tlists for the children need to have their wholerows transformed into the > corresponding ConvertRowtypeExprs *at this point*, so what I'd like to > propose is to 1) map a parent wholerow Var simply to a child wholerow Var, > instead (otherwise, the same as adjust_appendrel_attrs), when building the > reltarget for a child in set_append_rel_size, 2) build the tlists for child > joins using those children's wholerow Vars at path creation time, and 3) > adjust those wholerow Vars in the tlists for subpaths in the chosen > AppendPath so that those are transformed into the corresponding > ConvertRowtypeExprs, at plan creation time (ie, in > create_append_plan/create_merge_append_plan). IIUC, this would not require > any changes to pull_var_clause as proposed by the patch. This would not > require any changes to postgres_fdw as proposed by the patch, either. In > addition, this would make unnecessary the code added to setrefs.c by the > partitionwise-join patch. Maybe I'm missing something though. Not translating whole-row expressions to ConvertRowtypeExpr before creating paths can lead to a number of anomalies. For example, 1. if there's an index on the whole-row expression of child, translating parent's whole-row expression to child's whole-row expression as is will lead to using that index, when in reality the it can not be used since the condition/ORDER BY clause (which originally referred the parent's whole-row expression) require the child's whole-row reassembled as parent's whole-row. 2. Constraints on child'd whole-row expression, will be used to prune a child when they can not be used since the original condition was on parent' whole-row expression and not that of the child. 3. Equivalence classes will think that a child whole-row expression (without ConvertRowtypeExpr) is equivalent to an expression which is part of the same equivalence class as the parent' whole-row expression. Given these serious problems, I don't think we could afford not to cover a child whole-row reference in ConvertRowtypeExpr. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
