Thanks for having another look. On Wed, 27 Mar 2019 at 00:22, Julien Rouhaud <rjuju...@gmail.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
mergeappend_to_append_conversion_v15.patch
Description: Binary data
