Re: Merge Append Patch merged up to 85devel

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.

Heikki suggested I separate the Append and MergeAppend nodes into two executor
nodes. I had that half done in my tree but looking it over it leads to a lot
of duplicated code and a strange effect that there's on Path node but two
Executor nodes which seems strange. I'm not sure which way to go here but at
least for now I'm leaving it this way since it's less code to write. If we
want it the other way to commit then I'll do it.

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. I don't see a problem tossing
all the child relation attributes into the same eclass even though they're not
strictly speaking "equivalent". No join above the append path is going to see
the child attributes anyways. But that might be shortsighted as I'm not really
sure what the consequences are and what other uses we have envisioned for
eclasses in the future.

I've added this to the July commitfest.

Gregory Stark wrote:
> 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.
>
> Heikki suggested I separate the Append and MergeAppend nodes into two executor
> nodes. I had that half done in my tree but looking it over it leads to a lot
> of duplicated code and a strange effect that there's on Path node but two
> Executor nodes which seems strange. I'm not sure which way to go here but at
> least for now I'm leaving it this way since it's less code to write. If we
> want it the other way to commit then I'll do it.
>
> 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. I don't see a problem tossing
> all the child relation attributes into the same eclass even though they're not
> strictly speaking "equivalent". No join above the append path is going to see
> the child attributes anyways. But that might be shortsighted as I'm not really
> sure what the consequences are and what other uses we have envisioned for
> eclasses in the future.
>
>
>
> ------------------------------------------------------------------------
>
>
>
> ------------------------------------------------------------------------
>
>

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

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