Re: [HACKERS] Runtime Partition Pruning

On 22 December 2017 at 03:02, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
> This seems like having two different patches for the same feature. I
> will post my version of the patch which uses the struct
> PartitionPruneInfo from your patch and I will add the other additional
> features you added like optimizing the pruning rescan. I will try and
> post the patch tomorrow.

I apologise for persisting in making these parallel efforts. I do have
time right now to dedicate to review this patch, but that time is
running out. At this end of this time, I was really hoping that there
would be a patch that's worthy of being committed (or at least one
worthy of a committers time). During my review of v5, because I found
the patch to still need quite a bit of work, I thought the best use of
that time was to make it work myself, which to my knowledge I have
done. Although, I'm sure my patch will still have bugs, it appears to
me to be quite a bit further ahead than your v7 WIP patch.

> If there is more suggestions, you can give it over that; otherwise it
> seems like duplicating efforts.

Much of the things I did differently from you could be taken as suggestions.

There were a number of things in the v7 patch were still not in a
workable state:

1. Using the PlannerInfo to record details about Append. How will this
work with a plan containing multiple Appends scanning partitioned
tables?
2. The use of AppendState->subplan_indexes List. Please use a
Bitmapset to mark the valid subplans. Lists are not particularly
efficient to get the nth item.
3. Use of PlannerInfo to store details specific to a single
partitioned table in set_base_rel_sizes.
4. Use of a new PlannerInfo->join_clauses in set_rel_size(). How will
this work when there are multiple partitioned tables being scanned in
a single plan?
5. In match_clauses_to_partkey() you're using a new
PlannerInfo->baserestrictinfo_param_indexes List to store ParamIds.
How will this work when there are multiple partitioned tables being
scanned in a single plan? A Bitmapset would be a better choice to
store paramids in.
6. In set_append_rel_pathlist you're using more PlannerInfo members to
handle a specific Append rel. Again, how will it work for plans
scanning multiple different partitioned tables?
7. Your changes to get_appendrel_parampathinfo() ignore equivalence
join clauses, don't you need to look at these too? If so, maybe it
might be worth just using get_baserel_parampathinfo()?
8. Lack of ability to detect if set_append_subplan_indexes() needs to
be called in ExecReScanAppend(). Some parameters that change might not
have an effect on which partitions to scan.

If you go and find a new way to solve all those problems, then please
consider which one of us it is that's making the duplicate effort.

Again, I'm sorry that I have been standing on your toes with my work
here. I'm certainly not out to try to take any glory here. I just want
the patch to be in a working state and the time I have to do that is
fast running out.

Please consider my efforts as an offer of assistance rather than a
threat to your work.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services