Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [HACKERS] path toward faster partition pruning

Hi David.

On Wed, Jan 17, 2018 at 6:19 PM, David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> On 17 January 2018 at 17:05, David Rowley <david(dot)rowley(at)2ndquadrant(dot)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