RE: speeding up planning with partitions

Amit-san,

On Fri, Mar 1, 2019 at 1:02 PM, Amit Langote wrote:
> Thanks for testing and sorry it took me a while to reply.

Thanks for working about this late at night. I know you have a lot of things to do.

> On 2019/02/25 15:24, Imai, Yoshikazu wrote:
> > [update_pt_with_joining_another_pt.sql]
> > update rt set c = jrt.c + 100 from jrt where rt.b = jrt.b;
> >
> > [pgbench]
> > pgbench -n -f update_pt_with_joining_another_pt_for_ptkey.sql -T 60
> > postgres
> >
> > [results]
> > (part_num_rt, part_num_jrt)  master  patched(0001)
> > ---------------------------  ------  -------------
> >                   (3, 1024)    8.06           5.89
> >                   (3, 2048)    1.52           0.87
> >                   (6, 1024)    4.11           1.77
> >
> >
> >
> > With HEAD, we add target inheritance and source inheritance to
> parse->rtable in inheritance_planner and copy and adjust it for child
> tables at beginning of each planning of child tables.
> >
> > With the 0001 patch, we add target inheritance to parse->rtable in
> inheritance_planner and add source inheritance to parse->rtable in
> make_one_rel(under grouping_planner()) during each planning of child
> tables.
> > Adding source inheritance to parse->rtable may be the same process
> between each planning of child tables and it might be useless. OTOH, from
> the POV of making inheritance-expansion-at-bottom better, expanding
> source inheritance in make_one_rel seems correct design to me.
> >
> > How should we do that...?
> 
> To solve this problem, I ended up teaching inheritance_planner to reuse
> the objects for *source* inheritance child relations (RTEs,
> AppendRelInfos, and PlanRowMarks) created during the planning of the 1st
> child query for the planning of subsequent child queries.  Set of source
> child relations don't change between different planning runs, so it's
> okay to do so.  Of course, I had to make sure that query_planner (which
> is not in the charge of adding source inheritance child objects) can notice
> that.

I did above test again with v25 patch and checked the problem is solved.

[results]
(part_num_rt, part_num_jrt)  master  patched(0001)
---------------------------  ------  -------------
                  (3, 1024)    6.11           6.82
                  (3, 2048)    1.05           1.48
                  (6, 1024)    3.05           3.45

Sorry that I haven't checked the codes related this problem yet, but I'll check it later.


> Please find attached updated patches.  Will update source code comments,
> commit message and perform other fine-tuning over the weekend if possible.

I've taken at glance the codes and there are some minor comments about the patch.

* You changed a usage/arguments of get_relation_info, but why you did it? I made those codes back to the original code
andchecked it passes make check. So ISTM there are no logical problems with not changing it. Or if you change it, how
aboutalso change a usage/arguments of get_relation_info_hook in the same way?
 

-get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
-                  RelOptInfo *rel)
+get_relation_info(PlannerInfo *root, RangeTblEntry *rte, RelOptInfo *rel)
 {
+    bool        inhparent = rte->inh;
-    relation = table_open(relationObjectId, NoLock);
+    relation = heap_open(rte->relid, NoLock);
  ...
     if (get_relation_info_hook)
-        (*get_relation_info_hook) (root, relationObjectId, inhparent, rel);
+        (*get_relation_info_hook) (root, rte->relid, rte->inh, rel);


@@ -217,15 +272,13 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
     /* Check type of rtable entry */
     switch (rte->rtekind)
     {
         case RTE_RELATION:
             /* Table --- retrieve statistics from the system catalogs */
-            get_relation_info(root, rte->relid, rte->inh, rel);
+            get_relation_info(root, rte, rel);


* You moved the codes of initializing of append rel's partitioned_child_rels in set_append_rel_size() to
build_simple_rel(),but is it better to do? I also checked the original code passes make check by doing like above.
 

@@ -954,32 +948,6 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
     Assert(IS_SIMPLE_REL(rel));
 
     /*
-     * Initialize partitioned_child_rels to contain this RT index.
-     *
-     * Note that during the set_append_rel_pathlist() phase, we will bubble up
-     * the indexes of partitioned relations that appear down in the tree, so
-     * that when we've created Paths for all the children, the root
-     * partitioned table's list will contain all such indexes.
-     */
-    if (rte->relkind == RELKIND_PARTITIONED_TABLE)
-        rel->partitioned_child_rels = list_make1_int(rti);

@@ -274,55 +327,287 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
                                         list_length(rte->securityQuals));
 
     /*
-     * If this rel is an appendrel parent, recurse to build "other rel"
-     * RelOptInfos for its children.  They are "other rels" because they are
-     * not in the main join tree, but we will need RelOptInfos to plan access
-     * to them.
+     * Add the parent to partitioned_child_rels.
+     *
+     * Note that during the set_append_rel_pathlist() phase, values of the
+     * indexes of partitioned relations that appear down in the tree will
+     * be bubbled up into root parent's list so that when we've created
+     * Paths for all the children, the root table's list will contain all
+     * such indexes.
      */
-    if (rte->inh)
+    if (rel->part_scheme)
+        rel->partitioned_child_rels = list_make1_int(relid);

I'll review rest of codes.

--
Yoshikazu Imai