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

On Sat, Jul 14, 2018 at 2:41 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> On 2018-Jul-13, Ashutosh Bapat wrote:
>
>> 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.
>
> Hmm, let me reword this comment completely.  How about the attached?
>
> I also changed the order of the tests, putting the 'generate_opsteps'
> one in the middle instead of at the end (so the not-null one doesn't
> test that boolean again).  AFAICS it's logically the same, and it makes
> more sense to me that way.

That looks much better. However it took me a small while to understand
that (1), (2) and (3) correspond to strategies.

>
> I also changed the tests so that they apply to the existing mc2p table,
> which is identical to the one Amit was adding.

+1 for that.

>
>> > 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.
>
> I'm not sure I understand this question.  This strategy applies to hash
> partitioning only if we have null tests for all keys, so there are no
> op_exprs.  If there are any, there's no point in checking them.

With the rearranged code, it's much more simple to understand this
code. No change needed.

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

No need with the re-written code.

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