RE: speeding up planning with partitions - Mailing list pgsql-hackers

Amit-san,

On Tue, Mar 5, 2019 at 0:51 AM, Amit Langote wrote:
> Hi Jesper,
> 
> Thanks for the review.  I've made all of the changes per your comments
> in my local repository.  I'll post the updated patches after diagnosing
> what I'm suspecting a memory over-allocation bug in one of the patches.
> If you configure build with --enable-cassert, you'll see that throughput
> decreases as number of partitions run into many thousands, but it doesn't
> when asserts are turned off.
> 
> On 2019/03/05 1:20, Jesper Pedersen wrote:
> > I'll run some tests using a hash partitioned setup.
> 
> Thanks.

I've also done code review of 0001 and 0002 patch so far.

[0001]
1. Do we need to open/close a relation in add_appendrel_other_rels()? 

+    if (rel->part_scheme)
     {
-        ListCell   *l;
    ...
-        Assert(cnt_parts == nparts);
+        rel->part_rels = (RelOptInfo **)
+                palloc0(sizeof(RelOptInfo *) * rel->nparts);
+        relation = table_open(rte->relid, NoLock);
     }

+    if (relation)
+        table_close(relation, NoLock);


2. We can sophisticate the usage of Assert in add_appendrel_other_rels().

+    if (rel->part_scheme)
     {
    ...
+        rel->part_rels = (RelOptInfo **)
+                palloc0(sizeof(RelOptInfo *) * rel->nparts);
+        relation = table_open(rte->relid, NoLock);
     }
  ...
+    i  = 0;
+    foreach(l, root->append_rel_list)
+    {
    ...
+        if (rel->part_scheme != NULL)
+        {
+            Assert(rel->nparts > 0 && i < rel->nparts);
+            rel->part_rels[i] = childrel;
+        }
+
+        i++;

as below;

+    if (rel->part_scheme)
     {
    ...
    Assert(rel->nparts > 0);
+        rel->part_rels = (RelOptInfo **)
+                palloc0(sizeof(RelOptInfo *) * rel->nparts);
+        relation = table_open(rte->relid, NoLock);
     }
  ...
+    i  = 0;
+    foreach(l, root->append_rel_list)
+    {
    ...
+        if (rel->part_scheme != NULL)
+        {
+            Assert(i < rel->nparts);
+            rel->part_rels[i] = childrel;
+        }
+
+        i++;


[0002]
3. If using variable like is_source_inh_expansion, the code would be easy to read. (I might mistakenly understand
root->simple_rel_array_size> 0 means source inheritance expansion though.)
 

In expand_inherited_rtentry() and expand_partitioned_rtentry():

+         * Expand planner arrays for adding the child relations.  Can't do
+         * this if we're not being called from query_planner.
+         */
+        if (root->simple_rel_array_size > 0)
+        {
+            /* Needed when creating child RelOptInfos. */
+            rel = find_base_rel(root, rti);
+            expand_planner_arrays(root, list_length(inhOIDs));
+        }

+            /* Create the otherrel RelOptInfo too. */
+            if (rel)
+                (void) build_simple_rel(root, childRTindex, rel);

would be:

+         * Expand planner arrays for adding the child relations.  Can't do
+         * this if we're not being called from query_planner.
+         */
+        if (is_source_inh_expansion)
+        {
+            /* Needed when creating child RelOptInfos. */
+            rel = find_base_rel(root, rti);
+            expand_planner_arrays(root, list_length(inhOIDs));
+        }

+            /* Create the otherrel RelOptInfo too. */
+            if (is_source_inh_expansion)
+                (void) build_simple_rel(root, childRTindex, rel);


4. I didn't see much carefully, but should the introduction of contains_inherit_children be in 0003 patch...?


I'll continue to do code review of rest patches.


--
Yoshikazu Imai