From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
To: Imai, Yoshikazu <imai.yoshikazu@jp.fujitsu.com>
To: jesper.pedersen@redhat.com <jesper.pedersen@redhat.com>
Cc: 'Amit Langote' <amitlangote09@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Rowley <david.rowley@2ndquadrant.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: speeding up planning with partitions
Date: Wed, 6 Mar 2019 14:37:46 +0900
Message-ID: <03d9c823-4f65-e35c-9afa-7a1b42cc9917@lab.ntt.co.jp> (raw)
In-Reply-To: <0F97FA9ABBDBE54F91744A9B37151A5129BE1D@g01jpexmbkw24>
References: <CAKJS1f9XAePd2+8F4=wtHKdDdckgKcSoXb-ZbU8Wbt86mhLxKA@mail.gmail.com>
<b8a139b8-2f5d-6079-67b2-99957af3fc36@lab.ntt.co.jp>
<7453.1550518972@sss.pgh.pa.us>
<0a70234e-b2e6-debc-2fb5-e8fd927caa00@lab.ntt.co.jp>
<1002.1550596071@sss.pgh.pa.us>
<30961.1550609873@sss.pgh.pa.us>
<250c61bc-479c-92f4-d19d-d768e3bcdcbc@lab.ntt.co.jp>
<13510.1550677844@sss.pgh.pa.us>
<a47e6214-bd46-ab4b-2f84-f9f726e04642@lab.ntt.co.jp>
<35b02675-83c2-a12b-961a-fec249b06f35@lab.ntt.co.jp>
<20190222165932.GG28750@telsasoft.com>
<CA+HiwqEUwfg4s=4=is-+7k-WM-2Gs9K3iFB96QNm_eafcBozqQ@mail.gmail.com>
<0F97FA9ABBDBE54F91744A9B37151A51293422@g01jpexmbkw24>
<8098beec-fc04-303d-55b9-73c3d94ba1ea@lab.ntt.co.jp>
<b5362c29-cca1-de7b-13d0-9e51f9911185@lab.ntt.co.jp>
<ce7bb7c2-8eac-0047-1e04-e771f08ad1fb@redhat.com>
<0bb0f410-bfbc-3ee5-946b-43e89e8cbd57@lab.ntt.co.jp>
<ee0c1d07-e2bc-57af-6dd7-852eba7df61c@lab.ntt.co.jp>
<0F97FA9ABBDBE54F91744A9B37151A5129BE1D@g01jpexmbkw24>
Imai-san,
Thanks for the review.
On 2019/03/06 11:09, Imai, Yoshikazu wrote:
> Here is the code review for previous v26 patches.
>
> [0002]
> In expand_inherited_rtentry():
>
> expand_inherited_rtentry()
> {
> ...
> + RelOptInfo *rel = NULL;
>
> can be declared at more later:
>
> if (oldrelation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> ...
> else
> {
> List *appinfos = NIL;
> RangeTblEntry *childrte;
> Index childRTindex;
> + RelOptInfo *rel = NULL;
>
>
This is fixed in v27.
> [0003]
> In inheritance_planner:
>
> + rtable_with_target = list_copy(root->parse->rtable);
>
> can be:
>
> + rtable_with_target = list_copy(parse->rtable);
Sure.
> [0004 or 0005]
> There are redundant process in add_appendrel_other_rels (or expand_xxx_rtentry()?).
> I modified add_appendrel_other_rels like below and it actually worked.
>
>
> add_appendrel_other_rels(PlannerInfo *root, RangeTblEntry *rte, Index rti)
> {
> ListCell *l;
> RelOptInfo *rel;
>
> /*
> * Add inheritance children to the query if not already done. For child
> * tables that are themselves partitioned, their children will be added
> * recursively.
> */
> if (rte->rtekind == RTE_RELATION && !root->contains_inherit_children)
> {
> expand_inherited_rtentry(root, rte, rti);
> return;
> }
>
> rel = find_base_rel(root, rti);
>
> foreach(l, root->append_rel_list)
> {
> AppendRelInfo *appinfo = lfirst(l);
> Index childRTindex = appinfo->child_relid;
> RangeTblEntry *childrte;
> RelOptInfo *childrel;
>
> if (appinfo->parent_relid != rti)
> continue;
>
> Assert(childRTindex < root->simple_rel_array_size);
> childrte = root->simple_rte_array[childRTindex];
> Assert(childrte != NULL);
> build_simple_rel(root, childRTindex, rel);
>
> /* Child may itself be an inherited relation. */
> if (childrte->inh)
> add_appendrel_other_rels(root, childrte, childRTindex);
> }
> }
If you don't intialize part_rels here, then it will break any code in the
planner that expects a partitioned rel with non-zero number of partitions
to have part_rels set to non-NULL. At the moment, that includes the code
that implements partitioning-specific optimizations such partitionwise
aggregate and join, run-time pruning etc. No existing regression tests
cover the cases where source inherited roles participates in those
optimizations, so you didn't find anything that broke. For an example,
consider the following update query:
update p set a = p1.a + 1 from p p1 where p1.a = (select 1);
Planner will create a run-time pruning aware Append node for p (aliased
p1), for which it will need to use the part_rels array. Note that p in
this case is a source relation which the above code initializes.
Maybe we should add such regression tests.
> I didn't investigate that problem, but there is another memory increase
issue, which is because of 0003 patch I think. I'll try to solve the
latter issue.
Interested in details as it seems to be a separate problem.
Thanks,
Amit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: Langote_Amit_f8@lab.ntt.co.jp, imai.yoshikazu@jp.fujitsu.com, jesper.pedersen@redhat.com, amitlangote09@gmail.com, tgl@sss.pgh.pa.us, david.rowley@2ndquadrant.com, alvherre@2ndquadrant.com, pryzby@telsasoft.com
Subject: Re: speeding up planning with partitions
In-Reply-To: <03d9c823-4f65-e35c-9afa-7a1b42cc9917@lab.ntt.co.jp>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox