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

On Mon, 28 Jan 2019 at 21:21, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks for the fix, I'll incorporate it in the next version I'll post by
> tomorrow.

I just started reading 0001 again. I made a few notes:

1. Should bms_del_members() instead of bms_difference() if you don't
mind modifying in place, which it sounds like you don't, going by the
comment.

/*
* Now fix up EC's relids set.  It's OK to modify EC like this,
* because caller must have made a copy of the original EC.
* For example, see adjust_inherited_target_child_root.
*/
cur_ec->ec_relids = bms_difference(cur_ec->ec_relids,
   parent_rel->relids);
cur_ec->ec_relids = bms_add_members(cur_ec->ec_relids,
child_rel->relids);

2.  In the comment:

/*
* Now fix up EC's relids set.  It's OK to modify EC like this,
* because caller must have made a copy of the original EC.
* For example, see adjust_inherited_target_child_root.
*/

You mention that the caller must have made a copy, but the header
comment mentions nothing about that to warn callers.

3. Would it be better to do the following in make_rel_from_joinlist()?

/*
* Check that we got at least one usable path.  In the case of an
* inherited update/delete operation, no path has been created for
* the query's actual target relation yet.
*/
if (!root->inherited_update &&
(!final_rel ||
!final_rel->cheapest_total_path ||
final_rel->cheapest_total_path->param_info != NULL))
elog(ERROR, "failed to construct the join relation");

That way the test is also performed for each partition's join problem
and you don't need that weird special case to disable the check.

4. Do you think it would be nicer if
inheritance_make_rel_from_joinlist returned rel?

inheritance_make_rel_from_joinlist(root, joinlist);

/*
* Return the RelOptInfo of original target relation, although this
* doesn't really contain the final path.  inheritance_planner
* from where we got here will generate the final path, but it will
* do so by iterative over child subroots, not through this
* RelOptInfo.
*/
rel = find_base_rel(root, root->parse->resultRelation);

5. "iterative over child subroots" -> "iterating over the child subroots".

6. In set_inherit_target_rel_sizes() the
!bms_is_member(appinfo->child_relid, live_children) check could be
moved much earlier, likely just after the if (appinfo->parent_relid !=
parentRTindex) check.

I understand you're copying set_append_rel_size() here, but I don't
quite understand the reason why that function is doing it that way.
Seems like wasted effort building the quals for pruned partitions.

7. In set_inherit_target_rel_sizes() I still don't really like the way
you're adjusting the EquivalenceClasses. Would it not be better to
invent a function similar to adjust_appendrel_attrs(), or just use
that function?

8. Still a typo in this comment:

+ * ass dummy.  We must do this in this phase so that the rel's

9. "parent's" -> "the parent's"

/* Also propagate this child's own children into parent's list. */

10. Too many "of":

 * For each child of of the query's result relation, this translates the

11. Badly formed multiline comment.

/* Save the just translated targetlist as unexpanded_tlist in the child's
* subroot, so that this child's own children can use it.  Must use copy

12. DEELETE -> DELETE

/*
* The following fields are set during query planning portion of an
* inherited UPDATE/DEELETE operation.
*/

13. Surely this is not true?

* non-NULL.  Content of each PlannerInfo is same as the parent
* PlannerInfo, except for the parse tree which is a translated copy of
* the parent's parse tree.

Are you not modifying the EquivalenceClasses?

14. This comment plus the variable name is confusing me:

/*
* RelOptInfos corresponding to each child target rel.  For leaf children,
* it's the RelOptInfo representing the output of make_rel_from_joinlist()
* called with the parent rel in the original join tree replaced by a
* given leaf child.  For non-leaf children, it's the baserel RelOptInfo
* itself, left as a placeholder.
*/
List    *inh_target_child_joinrels;

The variable name mentions joinrels, but the comment target rels.  A
join rel can't be the target of an UPDATE/DELETE.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services