Horton

RE: speeding up planning with partitions

Imai, Yoshikazu <imai.yoshikazu@jp.fujitsu.com>

View thread View on postgr.es
From: "Imai, Yoshikazu" <imai.yoshikazu@jp.fujitsu.com>
To: 'Amit Langote' <Langote_Amit_f8@lab.ntt.co.jp>, Amit Langote <amitlangote09@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, David Rowley <david.rowley@2ndquadrant.com>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2019-02-08T04:44:16Z
Lists: pgsql-hackers 
Amit-san,

On Thu, Feb 7, 2019 at 10:22 AM, Amit Langote wrote:
> Rebased over bdd9a99aac.

I did code review of 0001 and I have some suggestions. Could you check them?

1.
0001: line 418
+		 * add_inherit_target_child_root() would've added only those that are

add_inherit_target_child_root() doesn't exist now, so an above comment needs to be modified.

2.
0001: line 508-510

In set_inherit_target_rel_pathlists():
+	/* Nothing to do if all the children were excluded. */
+	if (IS_DUMMY_REL(rel))
+		return;

These codes aren't needed or can be replaced by Assert because set_inherit_target_rel_pathlists is only called from set_rel_pathlist which excutes IS_DUMMY_REL(rel) before calling set_inherit_target_rel_pathlists, as follows.

set_rel_pathlist(...)
{
    ...
    if (IS_DUMMY_REL(rel))
    {    
        /* We already proved the relation empty, so nothing more to do */
    }    
    else if (rte->inh)
    {    
        /*
         * If it's a target relation, set the pathlists of children instead.
         * Otherwise, we'll append the outputs of children, so process it as
         * an "append relation".
         */
        if (root->inherited_update && root->parse->resultRelation == rti) 
        {
            inherited_update = true;
            set_inherit_target_rel_pathlists(root, rel, rti, rte);

3.
0001: line 1919-1920

-		case CONSTRAINT_EXCLUSION_ON:
-			break;				/* always try to exclude */

CONSTRAINT_EXCLUSION_ON is no longer used, so should we remove it also from guc parameters?

4.
0001:

Can we remove enum InheritanceKind which is no longer used?


I also see the patch from a perspective of separating codes from 0001 which are not essential of Overhaul inheritance update/delete planning. Although almost all of codes are related each other, but I found below two things can be moved to another patch.

---
0001: line 550-608

This section seems to be just refinement of set_append_rel_size().
So can we separate this from 0001 to another patch?

---
0001: line 812-841, 940-947, 1525-1536, 1938-1947 

These codes are related to removement of InheritanceKind from relation_excluded_by_constraints(), so I think it is something like cleaning of unneeded codes. Can we separate this to patch as some-code-clearnups-of-0001.patch? Of course, we can do that only if removing of these codes from 0001 would not bother success of "make check" of 0001.
I also think that what I pointed out at above 3. and 4. can also be included in some-code-clearnups-of-0001.patch.

What do you think?


--
Yoshikazu Imai

Commits

  1. Clean up handling of constraint_exclusion and enable_partition_pruning.

  2. Add test case exercising formerly-unreached code in inheritance_planner.

  3. Speed up planning when partitions can be pruned at plan time.

  4. Avoid crash in partitionwise join planning under GEQO.

  5. Avoid passing query tlist around separately from root->processed_tlist.

  6. Build "other rels" of appendrel baserels in a separate step.

  7. Get rid of duplicate child RTE for a partitioned table.

  8. Rearrange make_partitionedrel_pruneinfo to avoid work when we can't prune.

  9. Don't copy PartitionBoundInfo in set_relation_partition_info.

  10. Move building of child base quals out into a new function

  11. Reorganize planner code moved in b60c39759908

  12. Move inheritance expansion code into its own file

  13. Fix inherited UPDATE/DELETE with UNION ALL subqueries.

  14. Rearrange planner to save the whole PlannerInfo (subroot) for a subquery.