Re: [HACKERS] path toward faster partition pruning

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
Cc: Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Beena Emerson <memissemerson(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] path toward faster partition pruning
Date: 2018-02-21 10:44:08
Message-ID: 1f6498e8-377f-d077-e791-5dc84dba2c00@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v32-0001-Modify-bound-comparision-functions-to-accept-mem.patch text/plain 6.5 KB
v32-0002-Refactor-partition-bound-search-functions.patch text/plain 8.2 KB
v32-0003-Add-parttypid-partcollation-partsupfunc-to-Parti.patch text/plain 5.2 KB
v32-0004-Faster-partition-pruning.patch text/plain 111.3 KB
v32-0005-Add-only-unpruned-partitioned-child-rels-to-part.patch text/plain 23.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Laurenz Albe 2018-02-21 11:18:15 Re: NEXT VALUE FOR sequence
Previous Message Ashutosh Bapat 2018-02-21 10:42:03 Re: NEXT VALUE FOR sequence