Horton

Re: [HACKERS] path toward faster partition pruning

Amit Langote <langote_amit_f8@lab.ntt.co.jp>

View thread View on postgr.es
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, Robert Haas <robertmhaas@gmail.com>, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>, Jesper Pedersen <jesper.pedersen@redhat.com>, Amit Langote <amitlangote09@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Beena Emerson <memissemerson@gmail.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2018-02-21T10:44:08Z
Lists: pgsql-hackers

Attachments

Hi David.

Thanks for the review.

On 2018/02/21 19:15, David Rowley wrote:
> Thanks for fixing. I made a pass over v31 and only see a few small things:
> 
> 1. In get_partitions_for_keys() why is the
> get_partitions_excluded_by_ne_datums call not part of
> get_partitions_for_keys_list?

Hmm, there is a question of where exactly to put the call within
get_partitions_for_keys_list().  At the end would sound like an obvious
answer, but we tend to short-circuit return from that function at various
points, which it seems undesirable to change.  So, I left things as is here.

> 2. Still a stray "minoff += 1;" in get_partitions_for_keys_range

I actually found a few and changed them to ++ or --, as applicable.

> 
> 3. You're also preferring to minoff--/++, but maxoff -= 1/maxoff += 1;
> would be nice to see the style unified here.

Fixed all as mentioned above.

> 4. "other other"
> 
>  * that is, each of its fields other other than clauseinfo must be valid before

Fixed.

> 5. "a IS NULL" -> "an IS NULL":
> 
>  * Based on a IS NULL or IS NOT NULL clause that was matched to a partition

Fixed.

> 6. Can you add a warning in the header comment for
> extract_partition_clauses() to explain "Note: the 'clauses' List may
> be modified inside this function. Callers may like to make a copy of
> important lists before passing them to this function.", or something
> like that...

At least in my patch, extract_partition_clauses() is a local function with
just one caller, but I still don't see any problem with warning the
reader.  So, done.

> 7. "null" -> "nulls"
> 
> * Only allow strict operators.  This will guarantee null are
> 
> 8. "dicard" -> "discard"
> 
> * contains a <= 2, then because 3 <= 2 is false, we dicard a < 3 as

Fixed.


Please find attached updated patches.

Thanks,
Amit

Commits

  1. Fix assorted partition pruning bugs

  2. Make gen_partprune_steps static

  3. Remove useless 'default' clause

  4. Reorganize partitioning code

  5. Use custom hash opclass for hash partition pruning

  6. Blindly attempt to fix sepgsql tests broken due to 9fdb675fc5.

  7. Attempt to fix endianess issues in new hash partition test.

  8. Faster partition pruning

  9. For partitionwise join, match on partcollation, not parttypcoll.

  10. Revise API for partition bound search functions.

  11. Revise API for partition_rbound_cmp/partition_rbound_datum_cmp.

  12. Fix possible crash in partition-wise join.

  13. Refactor code for partition bound searching

  14. New C function: bms_add_range

  15. Add extensive tests for partition pruning.

  16. Add null test to partition constraint for default range partitions.

  17. Remove BufFile's isTemp flag.

  18. Make OWNER TO subcommand mention consistent

  19. Fix index matching for operators with mixed collatable/noncollatable inputs.