Re: speeding up planning with partitions

On 2019/01/29 11:23, David Rowley wrote:
> On Mon, 28 Jan 2019 at 21:21, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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:

Thanks.

> 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);

Fixed.

> 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.

OK, added a sentence.

> 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.

Moving it inside make_rel_from_joinlist() seems a bit awkward as there are
many return statements, with those appearing earlier in the function being
being for fast-path cases.

How about in the direct callers of make_rel_from_joinlist() viz.
make_one_rel() and inheritance_make_rel_from_joinlist()?

> 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);

OK, done. It now returns RelOptInfo root->parse->resultRelation.

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

Sentence no longer exists after above adjustments.

> 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.

Yeah, fixed that in both functions. Actually, as you might be aware, the
next patch obviates the need for this stanza at all, because pruned
partitions aren't added to root->append_rel_list.

> 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?

OK, I added a function copy_eq_classes_for_child_root() that simply makes
a copy of eq_classes from the source root (deep-copying where applicable)
and returns the list.

> 8. Still a typo in this comment:
>
> + * ass dummy. We must do this in this phase so that the rel's

Sorry, fixed.

> 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?

Yep, fixed. Also fixed items 9-12.

> 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.

How about inh_target_child_path_rels? The final ModifyTable's child
"subpaths" have to hang off of some RelOptInfo until we've done the final
grouping_planner() steps. Those RelOptInfos are stored here.

Attached updated patches.

Thanks,
Amit

PS: replies might be slow until Feb 5 (attending FOSDEM for the first time!)