Hi, On Thu, Nov 22, 2018 at 11:27 AM David Rowley <david.row...@2ndquadrant.com> wrote: > > On Mon, 5 Nov 2018 at 10:46, David Rowley <david.row...@2ndquadrant.com> > wrote: > > On 1 November 2018 at 22:05, Antonin Houska <a...@cybertec.at> wrote: > > > I think these conditions are too restrictive: > > > > > > /* > > > * Determine if these pathkeys match the partition order, or > > > reverse > > > * partition order. It can't match both, so only go to the > > > trouble of > > > * checking the reverse order when it's not in ascending partition > > > * order. > > > */ > > > partition_order = pathkeys_contained_in(pathkeys, > > > partition_pathkeys); > > > partition_order_desc = !partition_order && > > > pathkeys_contained_in(pathkeys, > > > > > > partition_pathkeys_desc); > > > > > > The problem with doing that is that if the partition keys are better > > than the pathkeys then we'll most likely fail to generate any > > partition keys at all due to lack of any existing eclass to use for > > the pathkeys. It's unsafe to use just the prefix in this case as the > > eclass may not have been found due to, for example one of the > > partition keys having a different collation than the required sort > > order of the query. In other words, we can't rely on a failure to > > create the pathkey meaning that a more strict sort order is not > > required. > > I had another look at this patch and it seems okay just to add a new > flag to build_partition_pathkeys() to indicate if the pathkey List was > truncated or not. In generate_mergeappend_paths() we can then just > check that flag before checking if the partiiton pathkeys are > contained in pathkeys. It's fine if the partition keys were truncated > for the reverse of that check. > > I've done this in the attached and added additional regression tests > for this case.
I started to look at v5. If I understand correctly, the new behavior is controlled by partitions_are_ordered(), but it only checks for declared partitions, not partitions that survived pruning. Did I miss something or is it the intended behavior? Also, generate_mergeappend_paths should probably be renamed to something like generate_sortedappend_paths since it can now generate either Append or MergeAppend paths. I'm also wondering if that's ok to only generate either a (sorted) Append or a MergeAppend. Is it possible that in some cases it's better to have a MergeAppend rather than a sorted Append, given that MergeAppend is parallel-aware and the sorted Append isn't?
