On 2018/07/12 14:32, Amit Langote wrote:
> 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 <[email protected]> wrote:
>>> On Wed, Jul 11, 2018 at 5:10 PM Dilip Kumar <[email protected]> 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.
I updated regression tests to reduce lines. There is no point in
repeating tests like v2 patch did.
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..a3b21130e3 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -3125,3 +3125,51 @@ 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);
+create table pp_multirange_def partition of pp_multirange default;
+-- all partitions but the default one should be pruned
+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..cf56fdac40 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -823,3 +823,19 @@ 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);
+create table pp_multirange_def partition of pp_multirange default;
+
+-- all partitions but the default one should be 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;
+
+drop table pp_multirange;
