Re: [HACKERS] path toward faster partition pruning

Hi Ashutosh.

On 2018/02/09 14:09, Ashutosh Bapat wrote:
> On Wed, Feb 7, 2018 at 7:17 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Feb 7, 2018 at 8:37 AM, Ashutosh Bapat
>> <ashutosh.bapat@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.

Thanks for the analysis.  I agree with all the points of concern.  so for
now, I have dropped all the changes from my patch that give rise to the
concerns.  With the new patch, changes to the existing optimizer code
beside introducing partprune.c in the util directory are pretty thin:

git diff master --stat src/backend/optimizer/
 src/backend/optimizer/path/allpaths.c  |   16 ++
 src/backend/optimizer/util/Makefile    |    2 +-
 src/backend/optimizer/util/clauses.c   |    4 +-
 src/backend/optimizer/util/partprune.c | 1421 +++++++++++
 src/backend/optimizer/util/plancat.c   |   83 ++++---
 src/backend/optimizer/util/relnode.c   |    8 +
 6 files changed, 1504 insertions(+), 30 deletions(-)

So, no refactoring the existing optimizer code, just replacing the
partition pruning mechanism with partprune.c functions.

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

I removed part_appinfos from the patch.  Also, I have made the changes
introducing live_partitioned_rels a separate patch, which we can discuss
independently of the pruning changes.

Will post the latest patch set later in the evening.

Thanks,
Amit