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
