Re: [HACKERS] path toward faster partition pruning - Mailing list pgsql-hackers

            On 15 February 2018 at 18:57, Amit Langote
<Langote_Amit_f8@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