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

From: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Date: 2017-02-27 14:59:01
Views: Raw Message | Whole Thread | Download mbox
Lists: pgsql-hackers

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:
The Russian Postgres Company

Attachment Content-Type Size
incremental-sort-2.patch application/octet-stream 108.9 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2017-02-27 15:07:08 Re: Proposal : For Auto-Prewarm.
Previous Message Magnus Hagander 2017-02-27 14:59:00 Re: Automatic cleanup of oldest WAL segments with pg_receivexlog