RE: speeding up planning with partitions

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