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