Re: partition pruning doesn't work with IS NULL clause in multikeyrange partition case - Mailing list pgsql-hackers

Thanks for the review.

On 2018/07/12 22:01, Ashutosh Bapat wrote:
> On Thu, Jul 12, 2018 at 11:10 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>
>>> 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.
> 
> +     *
> +     * 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.
> 
> I don't think this is true. When equality conditions and IS NULL clauses cover
> all partition keys of a hash partitioned table and do not have contradictory
> clauses, we should be able to find the partition which will remain unpruned.

I was trying to say the same thing, but maybe the comment doesn't like it.
 How about this:

+     * For hash partitioning, if we found IS NULL clauses for some keys and
+     * OpExpr's for others, gen_prune_steps_from_opexps() will generate the
+     * necessary pruning steps if all partition keys are taken care of.
+     * If we found only IS NULL clauses, then we can generate the pruning
+     * step here but only if all partition keys are covered.

> I
> see that we already have this supported in get_matching_hash_bounds()
>     /*
>      * For hash partitioning we can only perform pruning based on equality
>      * clauses to the partition key or IS NULL clauses.  We also can only
>      * prune if we got values for all keys.
>      */
>     if (nvalues + bms_num_members(nullkeys) == partnatts)
>     {
> 
>       */
> -    if (!generate_opsteps)
> +    if (!bms_is_empty(nullkeys) &&
> +        (part_scheme->strategy != PARTITION_STRATEGY_HASH ||
> +         bms_num_members(nullkeys) == part_scheme->partnatts))
> 
> So, it looks like we don't need bms_num_members(nullkeys) ==
> part_scheme->partnatts there.

Yes, we can perform pruning in all three cases for hash partitioning:

* all keys are covered by OpExprs providing non-null keys

* some keys are covered by IS NULL clauses, others by OpExprs (all keys
  covered)

* all keys are covered by IS NULL clauses

... as long as we generate a pruning step at all.  The issue here was that
we skipped generating the pruning step due to poorly coded condition in
gen_partprune_steps_internal in some cases.

> Also, I think, we don't know how some new partition strategy will treat NULL
> values so above condition looks wrong to me. Instead it should explicitly check
> the strategies for which we know that the NULL values go to a single partition.

How about if we explicitly spell out the strategies like this:

+    if (!bms_is_empty(nullkeys) &&
+        (part_scheme->strategy == PARTITION_STRATEGY_LIST ||
+         part_scheme->strategy == PARTITION_STRATEGY_RANGE ||
+         (part_scheme->strategy == PARTITION_STRATEGY_HASH &&
+          bms_num_members(nullkeys) == part_scheme->partnatts)))

The proposed comment also describes why the condition looks like that.

>          /*
> -         * 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.
> +         * There are no OpExpr's, but there are IS NOT NULL clauses, which
> +         * can be used to eliminate the null-partition-key-only partition.
> 
> I don't understand this. When there are IS NOT NULL clauses for all the
> partition keys, it's only then that we could eliminate the partition containing
> NULL values, not otherwise.

Actually, there is only one case where the pruning step generated by that
block of code is useful -- to prune a list partition that accepts only
nulls.  List partitioning only allows one partition, key so this works,
but let's say only accidentally.  I modified the condition as follows:

+    else if (!generate_opsteps && !bms_is_empty(notnullkeys) &&
+             bms_num_members(notnullkeys) == part_scheme->partnatts)

Attached updated patch.

Thanks,
Amit