Re: [HACKERS] path toward faster partition pruning

On 22 December 2017 at 13:57, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Will post the fixed version shortly, thanks.

I've made another partial pass on the patch and have a few more
things. #3 and #4 are just comments rather than requests to change
something. I think we should change those before PG11 though.

1. If I look at the elog(ERROR) messages in partition.c, there are a
number of variations of reporting an invalid partition strategy.

There seem to be 3 variations of the same thing. Probably the
"unexpected" one would suit most, but I've not looked too closely.

elog(ERROR, "invalid partitioning strategy");
elog(ERROR, "unexpected partition strategy: %d", (int) key->strategy);
elog(ERROR, "invalid partition strategy %c",
RelationGetPartitionKey(rel)->strategy);

2. In get_relation_constraints(). Can you add a comment to why you've added:

/* Append partition predicates, if any */
if (root->parse->commandType != CMD_SELECT)
{

I guess it must be because we use the new partition pruning code for
SELECT, but not for anything else.

3. It's a shame that RelOptInfo->live_partitioned_rels is a List and
not a RelIds. I guess if you were to change that you'd need to also
change AppendPath->partitioned_rels too, so probably we can just fix
that later.

4. live_part_appinfos I think could be a Relids type too, but probably
we can change that after this patch. Append subpaths are sorted in
create_append_path() for parallel append, so the order of the subpaths
seems non-critical.

5. Small memory leaks in get_partitions_from_clauses_recurse().

if (ne_clauses)
result = bms_int_members(result,
get_partitions_from_ne_clauses(relation,
ne_clauses));

Can you assign the result of get_partitions_from_ne_clauses() and
bms_free() it after the bms_int_members() ?

Same for:

result = bms_int_members(result,
get_partitions_from_or_clause_args(relation,
rt_index,
or->args));

The reason I'm being particular about this is that for the run-time
pruning patch we'll call this from ExecReScanAppend() which will
allocate into the ExecutorState which lives as long as the query does.
So any leaks will last the entire length of the query.
ExecReScanAppend() could be called millions of billions of times, so
we need to be sure that's not going to be a problem.

6. Similar to #5, memory leaks in get_partitions_from_or_clause_args()

arg_partset = get_partitions_from_clauses_recurse(relation, rt_index,
arg_clauses);

/*
* Partition sets obtained from mutually-disjunctive clauses are
* combined using set union.
*/
result = bms_add_members(result, arg_partset);

Need to bms_free(arg_partset)

Running out of time for today, but will look again in about 4 days.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services