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

On Fri, Jul 13, 2018 at 1:15 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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