On Wed, Feb 7, 2018 at 7:17 PM, Robert Haas <robertmh...@gmail.com> wrote:
> On Wed, Feb 7, 2018 at 8:37 AM, Ashutosh Bapat
> <ashutosh.ba...@enterprisedb.com> wrote:
>> While looking at the changes in partition.c I happened to look at the
>> changes in try_partition_wise_join(). They mark partitions deemed
>> dummy by pruning as dummy relations. If we accept those changes, we
>> could very well change the way we handle dummy partitioned tables,
>> which would mean that we also revert the recent commit
>> f069c91a5793ff6b7884120de748b2005ee7756f. But I guess, those changes
>> haven't been reviewed yet and so not final.
>
> Well, if you have an opinion on those proposed changes, I'd like to hear it.
I am talking about changes after this comment
/*
+ * If either child_rel1 or child_rel2 is not a live partition, they'd
+ * not have been touched by set_append_rel_size. So, its RelOptInfo
+ * would be missing some information that set_append_rel_size sets for
+ * live partitions, such as the target list, child EQ members, etc.
+ * We need to make the RelOptInfo of even the dead partitions look
+ * minimally valid and as having a valid dummy path attached to it.
+ */
There are couple of problems with this change
1. An N way join may call try_partition_wise_join() with the same base
relation on one side N times. The condition will be tried those many
times.
2. We will have to adjust or make similar changes in
try_partition_wise_aggregate() proposed in the partition-wise
aggregate patch. Right now it checks if the relation is dummy but it
will have to check whether the pathlist is also NULL. Any
partition-wise operation that we try in future will need this
adjustment.
AFAIU, for pruned partitions, we don't set necessary properties of the
corresponding RelOptInfo when it is pruned. If we were sure that we
will not use that RelOptInfo anywhere in the rest of the planning,
this would work. But that's not the case. AFAIU, current planner
assumes that a relation which has not been eliminated before planning
(DEAD relation), but later proved to not contribute any rows in the
result, is marked dummy. Partition pruning breaks that assumption and
thus may have other side effects, that we do not see right now. We
have similar problem with dummy partitioned tables, but we have code
in place to avoid looking at the pathlists of their children by not
considering such a partitioned table as partitioned. May be we want to
change that too.
Either we add refactoring patches to change the planner so that it
doesn't assume something like that OR we make sure that the pruned
partition's RelOptInfo have necessary properties and a dummy pathlist
set. I will vote for second. We spend CPU cycles marking pruned
partitions as dummy if the dummy pathlist is never used. May be we can
avoid setting dummy pathlist if we can detect that a pruned partition
is guaranteed not to be used, e.g when the corresponding partitioned
relation does not participate in any join or other upper planning.
Apart from that another change that caught my eye is
Instead of going through root->append_rel_list to pick up the child
appinfos, store them in an array called part_appinfos that stores
partition appinfos in the same order as RelOptInfos are stored in
part_rels, right when the latter are created.
Further, instead of going through root->pcinfo_list to get the list
of partitioned child rels, which ends up including even the rels
that are pruned by set_append_rel_size(), build up a list of "live"
partitioned child rels and use the same to initialize partitioned_rels
field of AppendPath.
That was voted down by Robert during partition-wise join
implementation. And I agree with him. Any changes around changing that
should change the way we handle AppendRelInfos for all relations, not
just (declarative) partitioned relations.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
