Hi David. On Wed, Jan 17, 2018 at 6:19 PM, David Rowley <david.row...@2ndquadrant.com> wrote: > On 17 January 2018 at 17:05, David Rowley <david.row...@2ndquadrant.com> > wrote: >> 6. Which brings me to; why do we need match_clauses_to_partkey at all? >> classify_partition_bounding_keys seems to do all the work >> match_clauses_to_partkey does, plus more. Item #3 above is caused by >> an inconsistency between these functions. What benefit does >> match_clauses_to_partkey give? I might understand if you were creating >> list of clauses matching each partition key, but you're just dumping >> everything in one big list which causes >> classify_partition_bounding_keys() to have to match each clause to a >> partition key again, and classify_partition_bounding_keys is even >> coded to ignore clauses that don't' match any key, so it makes me >> wonder what is match_clauses_to_partkey actually for? > > I started to look at this and ended up shuffling the patch around a > bit to completely remove the match_clauses_to_partkey function. > > I also cleaned up some of the comments and shuffled some fields around > in some of the structs to shrink them down a bit. > > All up, this has saved 268 lines of code in the patch. > > src/backend/catalog/partition.c | 296 ++++++++++++++++----------- > src/backend/optimizer/path/allpaths.c | 368 ++-------------------------------- > 2 files changed, 198 insertions(+), 466 deletions(-) > > It's had very minimal testing. Really I've only tested that the > regression tests pass. > > I also fixed up the bad assumption that IN lists will contain Consts > only which hopefully fixes the crash I reported earlier. > > I saw you'd added a check to look for contradicting IS NOT NULL > clauses when processing an IS NULL clause, but didn't do anything for > the opposite case. I added code for this so it behaves the same > regardless of the clause order. > > Can you look at my changes and see if I've completely broken anything?
Thanks for the patch. I applied the patch and see that it didn't break any tests, although haven't closely reviewed the code yet. I'm concerned that after your patch to remove match_clauses_to_partkey(), we'd be doing more work than necessary in some cases. For example, consider the case of using run-time pruning for nested loop where the inner relation is a partitioned table. With the old approach, get_partitions_from_clauses() would only be handed the clauses that are known to match the partition keys (which most likely is fewer than all of the query's clauses), so get_partitions_from_clauses() doesn't have to do the work of filtering non-partition clauses every time (that is, for every outer row). That's why I had decided to keep that part in the planner. Thanks, Amit
