Re: [HACKERS] path toward faster partition pruning

On 15 February 2018 at 18:57, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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