On Sat, Jul 14, 2018 at 2:41 AM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > On 2018-Jul-13, Ashutosh Bapat wrote: > >> On Fri, Jul 13, 2018 at 1:15 PM, Amit Langote >> <langote_amit...@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
