Re: Merge Append Patch merged up to 85devel

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> Here's a copy of the merge-append patch that I sent months ago merged up to
> head. I haven't really added any additional functionality since then.

I looked at the planner part of this a little bit. I think that it's
confusing "an append that produces an ordered result" with "an append
that's doing a merge". Those concepts need to be kept separate, because
as soon as we have a real concept of partitioned tables it will be
possible to have known-ordered output from a simple append of indexscans
on the partition key. So you need an explicit flag to say "we'll do
a merge", not just rely on whether the path has pathkeys.

As your comments note, the current approach to deciding which ordered
paths to generate is pretty unworkable --- it won't scale nicely at all
for large numbers of child tables. One random idea is to take some
specific one of the children (the largest, probably) as the "leader"
and consider only ordered-appends generating the same pathkeys as are
available for the leader. I approve of the fact that the code will
consider force-sorting children that are missing a way to match the
pathkeys, but presumably we don't want to do that on any but small
tables, so this seems like a possibly usable approach.

Speaking of sorting, it's not entirely clear to me how the patch ensures
that all the child plans produce the necessary sort keys as output
columns, and especially not how it ensures that they all get produced in
the *same* output columns. This might accidentally manage to work
because of the "throwaway" call to make_sort_from_pathkeys(), but at the
very least that's a misleading comment.

> The other pending question is the same I had back when I originally submitted
> it. I don't really understand what's going on with eclasses and what
> invariants we're aiming to maintain with them.

For non-equivalence-class qualifications, we translate the parent rel's
quals to match the child's columns using adjust_appendrel_attrs().
(This isn't necessarily a no-op because child rels might have different
physical numbers for inherited columns.) The child-eclass stuff is just
a mechanism to be able to generate suitably translated quals for the
cases where quals are being deduced from eclasses instead of presented
directly. I don't remember offhand whether there are any special
considerations for the associated pathkeys, but it seems possible that
it would Just Work --- especially if the patch did anything useful for
you at all ;-). In any case, I'm amazed that it's not failing
regression tests all over the place with those critical tests in
make_sort_from_pathkeys lobotomized by random #ifdef FIXMEs. Perhaps
we need some more regression tests...

In the same vein, the hack to "short circuit" the append stuff for
a single child node is simply wrong, because it doesn't allow for column
number variances. Please remove it.

regards, tom lane