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

On Mon, Jun 11, 2018 at 4:05 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> (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.

What happens to a PlaceHolderVar containing whole-row reference when
that appears in a condition and/or targetlist.

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

Yes. 1 will affect the choice of index-only scans. We use pathkey
comparisons at a number of places so tlist going out of sync with
equivalence classes can affect a number of places.

build_tlist_to_deparse() is used to deparse targetlist at the time of
creating paths,as well as during the planning. According to your
proposal we will build different tlists during path creation and plan
creation. That doesn't look good. Apart from that your proposal of
changing a child's targetlist at the time of plan creation to use CRE
doesn't help since the original problem as described in [1] happens at
the time of creating plans as described in [1].

The parent's whole-row reference has a different data type than
child's whole-row reference. If we do not cover child's whole-row
reference by CRE, the parent and child targetlists will go out of sync
as far as the type of individual columns are concerned. That too
doesn't look good to me.

[1] https://www.postgresql.org/message-id/CAFjFpRfD5=LsOqG1eUf-vMpm9KoEZBBGJo68D9wkUBJxop_emQ@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company