On 15 February 2018 at 18:57, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > Here is an updated version.
Thanks for sending v27. I've had a quick look over it while I was working on the run-time prune patch. However, I've not quite managed a complete pass of this version yet A couple of things so far: 1. Following loop; for (i = 0; i < partnatts; i++) { if (bms_is_member(i, keys->keyisnull)) { /* Only the default partition accepts nulls. */ if (partition_bound_has_default(boundinfo)) return bms_make_singleton(boundinfo->default_index); else return NULL; } } could become: if (partition_bound_has_default(boundinfo) && !bms_is_empty(keys->keyisnull) return bms_make_singleton(boundinfo->default_index); else return NULL; 2. Is the following form of loop necessary? for (i = 0; i < partnatts; i++) { if (bms_is_member(i, keys->keyisnull)) { keys->n_eqkeys++; keyisnull[i] = true; } } Can't this just be: i = -1; while ((i = bms_next_member(keys->keyisnull, i)) >= 0) { keys->n_eqkeys++; keyisnull[i] = true; } Perhaps you can just Assert(i < partnatts), if you're worried about that. Similar code exists in get_partitions_for_keys_range 3. Several comments mention partition_bound_bsearch, but there is now no such function. 4. "us" should be "is" * not be any unassigned range to speak of, because the range us unbounded 5. The following code is more complex than it needs to be: /* * Since partition keys with nulls are mapped to the default range * partition, we must include the default partition if some keys * could be null. */ if (keys->n_minkeys < partnatts || keys->n_maxkeys < partnatts) { for (i = 0; i < partnatts; i++) { if (!bms_is_member(i, keys->keyisnotnull)) { include_def = true; break; } } } Instead of the for loop, couldn't you just write: include_def = (bms_num_members(keys->keyisnotnull) < partnatts); 6. The following comment is not well written: * get_partitions_excluded_by_ne_datums * * Returns a Bitmapset of indexes of partitions that can safely be removed * due to each such partition's every allowable non-null datum appearing in * a <> opeartor clause. Maybe it would be better to write: * get_partitions_excluded_by_ne_datums * * Returns a Bitmapset of partition indexes that can safely be removed due to * the discovery of <> clauses for each datum value allowed in the partition. if not, then "opeartor" needs the spelling fixed. 7. "The following" * Followig entry points exist to this module. Are there any other .c files where we comment on all the extern functions in this way? I don't recall seeing it before. 8. The following may as well just: context.partnatts = partnatts; context.partnatts = rel->part_scheme->partnatts; 9. Why palloc0? Wouldn't palloc be ok? context.partkeys = (Expr **) palloc0(sizeof(Expr *) * context.partnatts); Also, no need for context.partnatts, just partnatts should be fine. 10. I'd rather see bms_next_member() used here: /* Add selected partitions' RT indexes to result. */ while ((i = bms_first_member(partindexes)) >= 0) result = bms_add_member(result, rel->part_rels[i]->relid); There's not really much use for bms_first_member these days. It can be slower due to having to traverse the unset lower significant bits each loop. bms_next_member starts where the previous loop left off. Will try to review more tomorrow. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services