Re: [PATCH] Incremental sort (was: PoC: Partial sort)

On Sun, Feb 19, 2017 at 2:18 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sat, Feb 18, 2017 at 4:01 PM, Alexander Korotkov
> <a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> > I decided to start new thread for this patch for following two reasons.
> > * It's renamed from "Partial sort" to "Incremental sort" per suggestion
> by
> > Robert Haas [1]. New name much better characterizes the essence of
> > algorithm.
> > * I think it's not PoC anymore. Patch received several rounds of review
> > and now it's in the pretty good shape.
> >
> > Attached revision of patch has following changes.
> > * According to review [1], two new path and plan nodes are responsible
> for
> > incremental sort: IncSortPath and IncSort which are inherited from
> SortPath
> > and Sort correspondingly. That allowed to get rid of set of hacks with
> > minimal code changes.
> > * According to review [1] and comment [2], previous tuple is stored in
> > standalone tuple slot of SortState rather than just HeapTuple.
> > * New GUC parameter enable_incsort is introduced to control planner
> ability
> > to choose incremental sort.
> > * Test of postgres_fdw with not pushed down cross join is corrected. It
> > appeared that with incremental sort such query is profitable to push
> down.
> > I changed ORDER BY columns so that index couldn't be used. I think this
> > solution is more elegant than setting enable_incsort = off.
>
> I usually advocate for spelling things out instead of abbreviating, so
> I guess I'll stay true to form here and suggest that abbreviating
> incremental to inc doesn't seem like a great idea. Is that sort
> incrementing, incremental, incredible, incautious, or incorporated?
>

I'm not that sure about naming of GUCs, because we already
have enable_hashagg instead of enable_hashaggregate, enable_material
instead of enable_materialize, enable_nestloop instead
of enable_nestedloop. But anyway I renamed "inc" to "Incremental"
everywhere in the code. I renamed enable_incsort GUC into
enable_incrementalsort as well, because I don't have strong opinion here.

The first hunk in the patch, a change in the postgres_fdw regression
> test output, looks an awful lot like a bug: now the query that
> formerly returned various different numbers is returning all zeroes.
> It might not actually be a bug, because you've also changed the test
> query (not sure why), but anyway the new regression test output that
> is all zeroes seems less useful for catching bugs in, say, the
> ordering of the results than the old output where the different rows
> were different.
>

Yes, I've changed regression test query as I mentioned in the previous
message. With incremental sort feature original query can't serve anymore
as an example of non-pushdown join. However, you're right that query which
returns all zeroes doesn't look good there either. So, I changed that
query to ordering by column "c3" which is actually non-indexed textual
representation of "c1".

> I don't know of any existing cases where the same executor file is
> responsible for executing more than 1 different type of executor node.
> I was imagining a more-complete separation of the new executor node.
>

Ok, I put incremental sort into separate executor node.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company