Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

(2018/06/07 19:42), Ashutosh Bapat wrote:
> On Wed, Jun 6, 2018 at 5:00 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> 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.

Yeah, but I mean we modify set_append_rel_size so that we only map a
parent wholerow Var in the parent tlist to a child wholerow Var in the
child's tlist; parent wholerow Vars in the parent's other expressions
such as conditions are transformed into CREs as-is.

>> 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.

Is that still possible when we only map a parent wholerow Var in the
parent's tlist to a child wholerow Var in the child's tlist?

Best regards,
Etsuro Fujita