Re: [HACKERS] path toward faster partition pruning

Hi David.

On 2017/12/22 10:35, David Rowley wrote:
> On 22 December 2017 at 13:57, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.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.

Thank you.

> 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);

I should have used the "unexpected ..." wording in the yesterday's update.
 Fixed.

> 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.

Yeah, I explained that a couple of times on email (maybe also in the
commit message), but not there.  Done.

> 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.

I agree.

> 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.

Hmm, perhaps.

> 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.

That's a very important point to stress.  Thanks.

> 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)

Fixed all these instances of leaks.

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

Thanks again.

Please find attached updated patches.

Thanks,
Amit