Re: Ordered Partitioned Table Scans

On Tue, Mar 26, 2019 at 3:13 AM David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>
> On Tue, 26 Mar 2019 at 09:02, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> > FTR this patch doesn't apply since single child [Merge]Append
> > suppression (8edd0e7946) has been pushed.
>
> Thanks for letting me know. I've attached v14 based on current master.

Thanks!

So, AFAICT everything works as intended, I don't see any problem in
the code and the special costing heuristic should avoid dramatic
plans.

A few, mostly nitpicking, comments:

+ if (rel->part_scheme != NULL && IS_SIMPLE_REL(rel) &&
+ partitions_are_ordered(root, rel))

shouldn't the test be IS_PARTITIONED_REL(rel) instead of testing
part_scheme? I'm thinking of 1d33858406 and related discussions.

+ * partitions_are_ordered
+ * For the partitioned table given in 'partrel', returns true if the
+ * partitioned table guarantees that tuples which sort earlier according
+ * to the partition bound are stored in an earlier partition. Returns
+ * false this is not possible, or if we have insufficient means to prove
+ * it.
[...]
+ * partkey_is_bool_constant_for_query
+ *
+ * If a partition key column is constrained to have a constant value by the
+ * query's WHERE conditions, then we needn't take the key into consideration
+ * when checking if scanning partitions in order can't cause lower-order
+ * values to appear in later partitions.

Maybe it's because I'm not a native english speaker, but I had to read
those comments multiple time. I'd also add to partitions_are_ordered
comments a note about default_partition (even though the function is
super short).

+ if (boundinfo->ndatums +
partition_bound_accepts_nulls(boundinfo) != partrel->nparts)
+ return false;

there are a few over lengthy lines, maybe a pgindent run would be useful.

+ * build_partition_pathkeys
+ * Build a pathkeys list that describes the ordering induced by the
+ * partitions of 'partrel'. (Callers must ensure that this partitioned
+ * table guarantees that lower order tuples never will be found in a
+ * later partition.). Sets *partialkeys to false if pathkeys were only
+ * built for a prefix of the partition key, otherwise sets it to true.
+ */
+List *
+build_partition_pathkeys(PlannerInfo *root, RelOptInfo *partrel,
+ ScanDirection scandir, bool *partialkeys)

Maybe add an assert partitions_are_ordered also?

And finally, should this optimisation be mentioned in ddl.sgml (or
somewhere else)?