Re: Ordered Partitioned Table Scans

Thanks for having another look.

On Wed, 27 Mar 2019 at 00:22, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> 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.

I don't think it's really needed. There must be > 0 partitions in this
case as if there were either 0 partitions or all partitions had been
pruned then the partitioned table would have been turned into a dummy
rel in set_append_rel_size(), and we'd never try to generate paths for
it. There are also quite a number of other places where we do the same
in add_paths_to_append_rel().

> + * 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've changed the wording of these a bit. I ended up aligning
partkey_is_bool_constant_for_query() with its cousin
indexcol_is_bool_constant_for_query(). Previously I'd tried to make
the comment contain a bit more detail about what calls it, but I've
now removed that part and replaced it with "then it's irrelevant for
sort-order considerations".

> I'd also add to partitions_are_ordered
> comments a note about default_partition (even though the function is
> super short).

hmm. The comments there do mention default partitions in each place
it's relevant. It's not relevant to mention anything about default
partitions in the header comment of the function since callers don't
need to know about implementation details. They just need details of
what the function does and what callers need to know. If we invent
some other naturally ordered partition strategy in the future that
does not allow default partitions then a comment in the function
header about default partitions would be not only irrelevant but also
confusing.

> + 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.

I've run pgindent. It won't wrap that line, so I wrapped it manually.
I don't think it's become any more pretty for it though.

> + * 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?

Added that.

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

I'm not too sure about this. We don't generally detail out planner
optimisations in the docs. However, maybe it's worth users knowing
about it as it may control their design choices of partition
hierarchies. I'd just not be sure where exactly something should be
written. I suppose ideally there'd be a section in the docs for
planner optimisations which could contain a section on partitioned
tables which we could reference from the partitioned table docs in
ddl.sgml. That would be asking a bit much for this patch though.

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