(2018/07/06 20:20), Ashutosh Bapat wrote:
On Fri, Jul 6, 2018 at 4:29 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
(2018/07/04 21:37), Ashutosh Bapat wrote:
On Wed, Jul 4, 2018 at 5:36 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.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
