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

Thanks for the review.

On 2019/02/08 18:27, David Rowley wrote:
> On Fri, 8 Feb 2019 at 22:12, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> 0001 is now a patch to remove duplicate code from set_append_rel_size.  It
>> combines multiple blocks that have the same body doing
>> set_dummy_rel_pathlist().
>>
>> 0002 is the "overhaul inherited update/delete planning"
> 
> I had started looking at v20's 0001.  I've not done a complete pass
> over it yet, but I'll likely just start again since v21 is out now:
> 
> I've removed the things you've fixed in v21. I think most of these
> will apply to the 0002 patch, apart form maybe #2.
> 
> 1. In set_rel_pathlist(), I wonder if you should be skipping the
> set_rel_pathlist_hook call when inherited_update is true.
> 
> Another comment mentions:
> 
> * not this rel.  Also, this rel's sole path (ModifyTable) will be set
> * by inheritance_planner later, so we can't check its paths yet.
> 
> So you're adding any paths for this rel

I've changed this part such that set_rel_pathlist returns right after the
last else{...} block if inherited_update is true.

> 2. The comment here mentions "partition", but it might just be a child
> of an inheritance parent:
> 
> if (childpruned ||
> !apply_child_basequals(root, rel, childrel, childRTE, appinfo) ||
> relation_excluded_by_constraints(root, childrel, childRTE))
> {
> /* This partition needn't be scanned; skip it. */
> set_dummy_rel_pathlist(childrel);
> continue;
> }
> 
> This occurs in both set_inherit_target_rel_sizes() and set_append_rel_size()

Fixed.

> 3. I think the following comment:
> 
> /* If the child itself is partitioned it may turn into a dummy rel. */
> 
> It might be better to have it as:
> 
> /*
> * If the child is a partitioned table it may have been marked
> * as a dummy rel from having all its partitions pruned.
> */
> 
> I mostly think that by saying "if the child itself is partitioned",
> then you're implying that we're currently operating on a partitioned
> table, but we might be working on an inheritance parent.

Agreed.  Your suggested wording is clearer, so rewrote the comment that way.

> 4. In set_inherit_target_rel_pathlists(), you have:
> 
> /*
> * If set_append_rel_size() decided the parent appendrel was
> * parallel-unsafe at some point after visiting this child rel, we
> * need to propagate the unsafety marking down to the child, so that
> * we don't generate useless partial paths for it.
> */
> if (!rel->consider_parallel)
> childrel->consider_parallel = false;
> 
> But I don't quite see why set_append_rel_size() would have ever been
> called for that rel. It should have been
> set_inherit_target_rel_sizes(). It seems like the entire chunk can be
> removed since set_inherit_target_rel_sizes() does not set
> consider_parallel.

This is a copy-pasted bit of code that's apparently useless.  Removed.

> 5. In planner.c you mention:
> 
> * inherit_make_rel_from_joinlist - this translates the jointree, replacing
>  * the target relation in the original jointree (the root parent) by
>  * individual child target relations and performs join planning on the
>  * resulting join tree, saving the RelOptInfos of resulting join relations
>  * into the top-level PlannerInfo
> 
> 
> I can't see a function named inherit_make_rel_from_joinlist().
> 
> 6. No such function:
> 
> * First, save the unexpanded version of the query's targetlist.
> * create_inherit_target_child_root will use it as base when expanding
> * it for a given child relation as the query's target relation instead
> * of the parent.
> */
> 
> and in:
> 
> /*
> * Add missing Vars to child's reltarget.
> *
> * create_inherit_target_child_root() would've added only those that
> * are needed to be present in the top-level tlist (or ones that
> * preprocess_targetlist thinks are needed to be in the tlist.)  We
> * may need other attributes such as those contained in WHERE clauses,
> * which are already computed for the parent during
> * deconstruct_jointree processing of the original query (child's
> * query never goes through deconstruct_jointree.)
> */

Oops, fixed.  So, these are the new functions:

set_inherit_target_rel_sizes
set_inherit_target_rel_pathlists

add_inherited_target_child_roots
create_inherited_target_child_root

inheritance_make_rel_from_joinlist

I've renamed set_inherit_target_* functions to set_inherited_target_* to
avoid having multiple styles of naming inheritance-related functions.

> 7. Where is "false" being passed here?
> 
> /* We haven't expanded inheritance yet, so pass false. */
> tlist = preprocess_targetlist(root);
> root->processed_tlist = tlist;
> qp_extra.tlist = tlist;
> qp_extra.activeWindows = NIL;
> qp_extra.groupClause = NIL;
> planned_rel = query_planner(root, tlist, standard_qp_callback, &qp_extra);

Hmm, thought I'd fixed this but maybe messed up again while rebasing.

> 8. Is this code in the wrong patch? I don't see any function named
> build_dummy_partition_rel in this patch.
>
> * Make child entries in the EquivalenceClass as well.  If the childrel
> * appears to be a dummy one (one built by build_dummy_partition_rel()),
> * no need to make any new entries, because anything that'd need those
> * can instead use the parent's (rel).
> */
> if (childrel->relid != rel->relid &&

Again, appears to be a rebasing mistake.  Moved that hunk to the "Lazy
creation of..." patch which necessitates it.

> 9. "to use" seems out of place here. It makes more sense if you remove
> those words.
>
> * Add child subroots needed to use during planning for individual child
> * targets

Removed.

> 10. Is this comment really needed?
>
> /*
> * This is needed, because inheritance_make_rel_from_joinlist needs to
> * translate root->join_info_list executing make_rel_from_joinlist for a
> * given child.
> */
>
> None of the other node types mention what they're used for.  Seems
> like something that might get outdated pretty quickly.

OK, removed.

> 11. adjust_appendrel_attrs_mutator: This does not seem very robust:
>
> /*
> * No point in searching if parent rel not mentioned in eclass; but we
> * can't tell that for sure if parent rel is itself a child.
> */
> for (cnt = 0; cnt < nappinfos; cnt++)
> {
> if (bms_is_member(appinfos[cnt]->parent_relid, ec->ec_relids))
> {
> appinfo = appinfos[cnt];
> break;
> }
> }
>
> What if the caller passes multiple appinfos and actually wants them
> all processed?  You'll only process the first one you find that has an
> eclass member.  I think you should just loop over each appinfo and
> process all the ones that have a match, not just the first.
>
> I understand the current caller only passes 1, but I don't think that
> gives you an excuse to take a shortcut on the implementation.  I think
> probably you've done this as that's what is done for Var in
> adjust_appendrel_attrs_mutator(), but a Var can only belong to a
> single relation. An EquivalenceClass can have members for multiple
> relations.

OK, I've refactored the code such that translation is carried out with
*all* appinfos passed to adjust_appendrel_attrs_mutator.

> 13. adjust_appendrel_attrs_mutator: This seems wrong:
>
> /*
> * We have found and replaced the parent expression, so done
> * with EC.
> */
> break;
>
> Surely there could be multiple members for the parent. Say:
>
> UPDATE parted SET ... WHERE x = y AND y = 1;

I hadn't considered that.  Removed the break so that *all* members of a
given EC are considered for translating.

> 14. adjust_appendrel_attrs_mutator: Comment is wrong. There's no
> function called adjust_inherited_target_child_root and the EC is
> copied in the function, not the caller.
>
> /*
> * 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.
> */
>
>
> 15. I don't think "Copy it before modifying though." should be part of
> this comment.
>
> /*
> * Adjust all_baserels to replace the original target relation with the
> * child target relation.  Copy it before modifying though.
> */
> subroot->all_baserels = adjust_child_relids(subroot->all_baserels,
> 1, &appinfo);

Updated both of these stale comments.

Attached updated patches.

Thanks,
Amit