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

(2018/07/06 20:20), Ashutosh Bapat wrote:
> On Fri, Jul 6, 2018 at 4:29 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> (2018/07/04 21:37), Ashutosh Bapat wrote:
>>> On Wed, Jul 4, 2018 at 5:36 PM, Etsuro Fujita
>>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:

>> Let me explain about that: 1) my patch won't apply that function to a child
>> if its top parent is an appendrel built from a UNION ALL subquery, even
>> though the child is a partition of a partitioned table pulled up from a leaf
>> subquery into the parent query, like lpt_p1, and 2) my patch will apply that
>> function to a child if its top parent is a partitioned table, whether or not
>> the partitioned table is involved in a partitionwise join. By #1, we avoid
>> the adjustment work at plan creation time, as explained above. It might be
>> worth trying to be smarter about #2 (for example, in the case of a join of a
>> partitioned table and a non-partitioned table, since we don't consider a
>> partitionwise join for that join, it's better to not apply that function to
>> partitions of the partitioned table, to avoid the adjustment work at plan
>> creation time), but ISTM we don't have enough information to be smarter.
>
> That looks like a kludge to me rather than a proper fix. It's not
> clear to me as to when somebody can expect ConvertRowtypeExpr in the
> targetlist and when don't while creating paths and to an extent plans.
> For example, inside add_paths_to_append_rel() or in
> apply_scanjoin_target_to_paths() or for that matter any path creation
> or plan creation function, we will sometimes get targetlists with
> ConvertRowtypeExpr() and sometime not. How do we know which is when.

That is known from a new flag "has_child_wholerow" added to RelOptInfo
to indicate whether an other relation's reltarget has child wholerow
Vars instead of ConvertRowtypeExprs. That flag is initialized to false
and only set to true in build_childrel_tlist if it adds child wholerow
Vars to the reltarget of a given other relation. Though, I don't think
we need to be conscious about whether the reltarget has child wholerow
Vars or ConvertRowtypeExprs when creating paths; we would just create
paths based on the reltarget. What we need to be careful about is when
creating an Append/MergeAppend plan; we have to adjust the subplan's
tlist so child wholerow Vars in that tlist are converted to the
corresponding ConvertRowtypeExprs, if that tlist has those Vars, which
is also known from the new flag.

>>>> I think the biggest issue in the original patch you proposed is that we
>>>> spend extra cycles where partitioning is not involved, which is the
>>>> biggest
>>>> reason why I think the original patch isn't the right way to go.
>>>
>>>
>>> When there are no partitions involved, there won't be any
>>> ConvertRowtypeExprs there which means the function
>>> is_converted_whole_row_reference() would just return from the first
>>> line checking IsA() and nullness of node.
>>
>>
>> Really? IIRC, with the original patch we would spend extra cycles in a
>> non-partitioned inheritance processing [1].
>
> As I said, we do spend cycles in that function testing whether a node
> is Aggref or not even when the query doesn't have aggregates or
> grouping OR spend cycles in testing whether a node is a PlaceHolderVar
> when the query doesn't produce any. So, I don't see any problem with
> spending a few cycles testing whether a node is ConvertRowtypeExpr or
> not when a ConvertRowtypeExpr is not in the query or command. That's
> not a huge performance trouble. I would be happy to change my mind, if
> you show me performance different with and without this patch in
> planning. I haven't seen any.

I have to admit that the case in [1] wouldn't affect the performance,
but my concern is that there might be some cases where the test affects
performance. In contrast, in the approach I proposed, we wouldn't need
to worry about that because the approach does not modify code that is
not related to partitioning. I think that's a good thing.

Best regards,
Etsuro Fujita