On 22 December 2017 at 03:02, Beena Emerson <memissemer...@gmail.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
