Horton

Re: speeding up planning with partitions

David Rowley <david.rowley@2ndquadrant.com>

View thread View on postgr.es
From: David Rowley <david.rowley@2ndquadrant.com>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Amit Langote <amitlangote09@gmail.com>, Alvaro Herrera <alvherre@2ndquadrant.com>, "Imai, Yoshikazu" <imai.yoshikazu@jp.fujitsu.com>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2019-01-10T09:12:46Z
Lists: pgsql-hackers 
On Thu, 10 Jan 2019 at 21:41, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> In the v13 that I will try to post tomorrow, I will have hopefully
> addressed David's and Imai-san's review comments.  Thank you both!

I'd been looking at v11's 0002 and started on 0003 too. I'll include
my notes so far if you're about to send a v13.


v11 0002

18. There's a missing case in the following code. I understand that
makeNode() will 0 the member here, but that does not stop the various
other initialisations that set the default value for the field.  Below
there's a missing case where parent != NULL && parent->rtekind !=
RTE_RELATION. You might be better just always zeroing the field below
"rel->partitioned_child_rels = NIL;"

+
+ /*
+ * For inheritance child relations, we also set inh_root_parent.
+ * Note that 'parent' might itself be a child (a sub-partitioned
+ * partition), in which case we simply use its value of
+ * inh_root_parent.
+ */
+ if (parent->rtekind == RTE_RELATION)
+ rel->inh_root_parent = parent->inh_root_parent > 0 ?
+ parent->inh_root_parent :
+ parent->relid;
  }
  else
+ {
  rel->top_parent_relids = NULL;
+ rel->inh_root_parent = 0;
+ }

19. Seems strange to have a patch that adds a new field that is
unused. I also don't quite understand yet why top_parent_relids can't
be used instead. I think I recall being confused about that before, so
maybe it's worth writing a comment to mention why it cannot be used.

v11 0003

20. This code looks wrong:

+ /*
+ * expand_inherited_tables may have proved that the relation is empty, so
+ * check if it's so.
+ */
+ else if (rte->inh && !IS_DUMMY_REL(rel))


Likely you'll want:

else if rte->inh)
{
if (IS_DUMMY_REL(rel))
return;
// other stuff
}

otherwise, you'll end up in the else clause when you shouldn't be.

21. is -> was

+ * The child rel's RelOptInfo is created during
+ * expand_inherited_tables().
  */
  childrel = find_base_rel(root, childRTindex);

since you're talking about something that already happened.

I'll continue looking at v12.

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

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.