Re: [HACKERS] path toward faster partition pruning

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
Date: 2017-12-22 04:25:30
Message-ID: 58c3e20a-a964-4fdb-4e7d-bd833e9bead1@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi David.

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.

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

Attachment Content-Type Size
0001-Some-interface-changes-for-partition_bound_-cmp-bsea-v17.patch text/plain 11.6 KB
0002-Introduce-a-get_partitions_from_clauses-v17.patch text/plain 63.8 KB
0003-Move-some-code-of-set_append_rel_size-to-separate-fu-v17.patch text/plain 8.6 KB
0004-More-refactoring-around-partitioned-table-AppendPath-v17.patch text/plain 13.1 KB
0005-Teach-planner-to-use-get_partitions_from_clauses-v17.patch text/plain 44.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
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]