Re: speeding up planning with partitions

On Fri, 8 Feb 2019 at 22:12, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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

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

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.

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.

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.)
*/

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

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