Re: partition pruning doesn't work with IS NULL clause in multikeyrange partition case

On Fri, Jul 13, 2018 at 1:15 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>
>> 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.
>

It's still confusing a bit. I think "taken care of" doesn't convey the
same meaning as "covered". I don't think the second sentence is
necessary, it talks about one special case of the first sentence. But
this is better than the prior version.

>> 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)))

I still do not understand why do we need (part_scheme->strategy ==
PARTITION_STRATEGY_HASH && bms_num_members(nullkeys) ==
part_scheme->partnatts) there. I thought that this will be handled by
code, which deals with null keys and op_exprs together, somewhere
else.

>
> 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)
>

Hmm. Ok. May be it's better to explicitly say that it's only useful in
case of list partitioned table.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company