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

(2018/06/14 22:37), Ashutosh Bapat wrote:
> 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:
>> 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.

Whole-row Vars contained in such PHV's expressions will be mapped into
ConvertRowtypeExprs with adjust_appendrel_attrs.

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

Sorry, I'm still not sure if #1 is really a problem. Could you show me
an example for that?

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

Maybe my explanation was not enough, but my proposal will guarantee that
the FDW can build the same tlist at path creation time and plan creation
time because the RelOptInfo's reltarget from which the tlist is created
doesn't change at all.

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

I think my proposal will address the original issue. Actually, I've
created a patch implementing that proposal. That's still WIP, but it
works well even for these cases (and makes code much more simple, as
mentioned above!) But I think that patch needs more work, so I'm
planning to post it next week.

Best regards,
Etsuro Fujita