Thanks Ashutosh for reporting and Dilip for the analysis and the patch.
On 2018/07/11 21:39, Dilip Kumar wrote:
> On Wed, Jul 11, 2018 at 5:36 PM, amul sul <sula...@gmail.com> wrote:
>> On Wed, Jul 11, 2018 at 5:10 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
>>>
>> I am not sure that I have understand the following comments
>> 11 + * Generate one prune step for the information derived from IS NULL,
>> 12 + * if any. To prune hash partitions, we must have found IS NULL
>> 13 + * clauses for all partition keys.
>> 14 */
>>
>> I am not sure that I have understood this -- no such restriction
>> required to prune the hash partitions, if I am not missing anything.
>
> Maybe it's not very clear but this is the original comments I have
> retained. Just moved it out of the (!generate_opsteps) condition.
>
> Just the explain this comment consider below example,
>
> create table hp (a int, b text) partition by hash (a int, b text);
> create table hp0 partition of hp for values with (modulus 4, remainder 0);
> create table hp3 partition of hp for values with (modulus 4, remainder 3);
> create table hp1 partition of hp for values with (modulus 4, remainder 1);
> create table hp2 partition of hp for values with (modulus 4, remainder 2);
>
> postgres=# insert into hp values (1, null);
> INSERT 0 1
> postgres=# insert into hp values (2, null);
> INSERT 0 1
> postgres=# select tableoid::regclass, * from hp;
> tableoid | a | b
> ----------+---+---
> hp1 | 1 |
> hp2 | 2 |
> (2 rows)
>
> Now, if we query based on "b is null" then we can not decide which
> partition should be pruned whereas in case
> of other schemes, it will go to default partition so we can prune all
> other partitions.
That's right. By generating a pruning step with only nullkeys set, we are
effectively discarding OpExprs that may have been found for some partition
keys. That's fine for list/range partitioning, because nulls can only be
found in a designated partition, so it's okay to prune all other
partitions and for that it's enough to generate the pruning step like
that. For hash partitioning, nulls could be contained in any partition so
it's not okay to discard OpExpr's like that. We can generate pruning
steps with combination of null and non-null keys in the hash partitioning
case if there are any OpExprs.
I think your fix is correct. I slightly modified it along with updating
nearby comments and added regression tests.
Thanks,
Amit
diff --git a/src/backend/partitioning/partprune.c
b/src/backend/partitioning/partprune.c
index cdc61a8997..d9612bf65b 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -854,44 +854,42 @@ gen_partprune_steps_internal(GeneratePruningStepsContext
*context,
}
/*
- * If generate_opsteps is set to false it means no OpExprs were directly
- * present in the input list.
+ * For list and range partitioning, null partition keys can only be
found
+ * in one designated partition, so if there are IS NULL clauses
containing
+ * partition keys we should generate a pruning step that gets rid of all
+ * partitions but the special null-accepting partitions. For range
+ * partitioning, that means we will end up disregarding OpExpr's that
may
+ * have been found for some other keys, but that's fine, because it is
not
+ * possible to prune range partitions with a combination of null and
+ * non-null keys.
+ *
+ * For hash partitioning however, it is possible to combine null and
non-
+ * null keys in a pruning step, so do this only if *all* partition keys
+ * are involved in IS NULL clauses.
*/
- if (!generate_opsteps)
+ if (!bms_is_empty(nullkeys) &&
+ (part_scheme->strategy != PARTITION_STRATEGY_HASH ||
+ bms_num_members(nullkeys) == part_scheme->partnatts))
+ {
+ PartitionPruneStep *step;
+
+ step = gen_prune_step_op(context, InvalidStrategy,
+ false, NIL,
NIL, nullkeys);
+ result = lappend(result, step);
+ }
+ else if (!generate_opsteps && !bms_is_empty(notnullkeys))
{
/*
- * Generate one prune step for the information derived from IS
NULL,
- * if any. To prune hash partitions, we must have found IS NULL
- * clauses for all partition keys.
+ * There are no OpExpr's, but there are IS NOT NULL clauses,
which
+ * can be used to eliminate the null-partition-key-only
partition.
*/
- if (!bms_is_empty(nullkeys) &&
- (part_scheme->strategy != PARTITION_STRATEGY_HASH ||
- bms_num_members(nullkeys) == part_scheme->partnatts))
- {
- PartitionPruneStep *step;
+ PartitionPruneStep *step;
- step = gen_prune_step_op(context, InvalidStrategy,
- false,
NIL, NIL, nullkeys);
- result = lappend(result, step);
- }
-
- /*
- * Note that for IS NOT NULL clauses, simply having step
suffices;
- * there is no need to propagate the exact details of which
keys are
- * required to be NOT NULL. Hash partitioning expects to see
actual
- * values to perform any pruning.
- */
- if (!bms_is_empty(notnullkeys) &&
- part_scheme->strategy != PARTITION_STRATEGY_HASH)
- {
- PartitionPruneStep *step;
-
- step = gen_prune_step_op(context, InvalidStrategy,
- false,
NIL, NIL, NULL);
- result = lappend(result, step);
- }
+ step = gen_prune_step_op(context, InvalidStrategy,
+ false, NIL,
NIL, NULL);
+ result = lappend(result, step);
}
- else
+ else if (generate_opsteps)
{
PartitionPruneStep *step;
diff --git a/src/test/regress/expected/partition_prune.out
b/src/test/regress/expected/partition_prune.out
index d15f1d37f1..ffee6a0b74 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -3125,3 +3125,89 @@ explain (costs off) select * from pp_temp_parent where a
= 2;
(3 rows)
drop table pp_temp_parent;
+-- multi-column range partitioning and pruning with IS NULL clauses on some
+-- columns
+create table pp_multirange (a int, b int) partition by range (a, b);
+create table pp_multirange1 partition of pp_multirange for values from (1, 1)
to (100, 100);
+create table pp_multirange2 partition of pp_multirange for values from (100,
100) to (200, 200);
+-- no default partition, so all of the following should result in both
+-- partitions being pruned
+explain (costs off) select * from pp_multirange where a = 1 and b is null;
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+explain (costs off) select * from pp_multirange where a is null and b is null;
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+explain (costs off) select * from pp_multirange where a is null and b = 1;
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+explain (costs off) select * from pp_multirange where a is null;
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+explain (costs off) select * from pp_multirange where b is null;
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+-- create default partition so that all of the above queries now scan only
+-- the default partition
+create table pp_multirange_def partition of pp_multirange default;
+explain (costs off) select * from pp_multirange where a = 1 and b is null;
+ QUERY PLAN
+-------------------------------------------
+ Append
+ -> Seq Scan on pp_multirange_def
+ Filter: ((b IS NULL) AND (a = 1))
+(3 rows)
+
+explain (costs off) select * from pp_multirange where a is null and b is null;
+ QUERY PLAN
+-----------------------------------------------
+ Append
+ -> Seq Scan on pp_multirange_def
+ Filter: ((a IS NULL) AND (b IS NULL))
+(3 rows)
+
+explain (costs off) select * from pp_multirange where a is null and b = 1;
+ QUERY PLAN
+-------------------------------------------
+ Append
+ -> Seq Scan on pp_multirange_def
+ Filter: ((a IS NULL) AND (b = 1))
+(3 rows)
+
+explain (costs off) select * from pp_multirange where a is null;
+ QUERY PLAN
+-------------------------------------
+ Append
+ -> Seq Scan on pp_multirange_def
+ Filter: (a IS NULL)
+(3 rows)
+
+explain (costs off) select * from pp_multirange where b is null;
+ QUERY PLAN
+-------------------------------------
+ Append
+ -> Seq Scan on pp_multirange_def
+ Filter: (b IS NULL)
+(3 rows)
+
+drop table pp_multirange;
diff --git a/src/test/regress/sql/partition_prune.sql
b/src/test/regress/sql/partition_prune.sql
index b8e823d562..1cc5cf31f4 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -823,3 +823,27 @@ create temp table pp_temp_part_def partition of
pp_temp_parent default;
explain (costs off) select * from pp_temp_parent where true;
explain (costs off) select * from pp_temp_parent where a = 2;
drop table pp_temp_parent;
+
+-- multi-column range partitioning and pruning with IS NULL clauses on some
+-- columns
+create table pp_multirange (a int, b int) partition by range (a, b);
+create table pp_multirange1 partition of pp_multirange for values from (1, 1)
to (100, 100);
+create table pp_multirange2 partition of pp_multirange for values from (100,
100) to (200, 200);
+
+-- no default partition, so all of the following should result in both
+-- partitions being pruned
+explain (costs off) select * from pp_multirange where a = 1 and b is null;
+explain (costs off) select * from pp_multirange where a is null and b is null;
+explain (costs off) select * from pp_multirange where a is null and b = 1;
+explain (costs off) select * from pp_multirange where a is null;
+explain (costs off) select * from pp_multirange where b is null;
+
+-- create default partition so that all of the above queries now scan only
+-- the default partition
+create table pp_multirange_def partition of pp_multirange default;
+explain (costs off) select * from pp_multirange where a = 1 and b is null;
+explain (costs off) select * from pp_multirange where a is null and b is null;
+explain (costs off) select * from pp_multirange where a is null and b = 1;
+explain (costs off) select * from pp_multirange where a is null;
+explain (costs off) select * from pp_multirange where b is null;
+drop table pp_multirange;
