|From:||Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>|
|Cc:||Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Beena Emerson <memissemerson(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: [HACKERS] path toward faster partition pruning|
|Views:||Raw Message | Whole Thread | Download mbox|
On 2017/12/22 10:35, David Rowley wrote:
> 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",
I should have used the "unexpected ..." wording in the yesterday's update.
> 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.
> 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,
> 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,
> 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,
> * 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.
Please find attached updated patches.
|Next Message||Kyotaro HORIGUCHI||2017-12-22 04:47:16||Re: Protect syscache from bloating with negative cache entries|
|Previous Message||Chapman Flack||2017-12-22 04:17:29||archive restore command failure status [was Re: patch proposal]|