Horton

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

Alexander Korotkov <a.korotkov@postgrespro.ru>

View thread View on postgr.es
From: Alexander Korotkov <a.korotkov@postgrespro.ru>
To: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2017-02-27T14:59:01Z
Lists: pgsql-hackers

Attachments

On Sun, Feb 19, 2017 at 2:18 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Sat, Feb 18, 2017 at 4:01 PM, Alexander Korotkov
> <a.korotkov@postgrespro.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

Commits

  1. Further adjustments to Hashagg EXPLAIN ANALYZE output

  2. Rework EXPLAIN format for incremental sort

  3. Fix typos and improve incremental sort comments

  4. Stabilize incremental_sort tests

  5. Minor improvements in Incremental Sort explain

  6. Consider Incremental Sort paths at additional places

  7. Fix representation of SORT_TYPE_STILL_IN_PROGRESS.

  8. Fix failures in incremental_sort due to number of workers

  9. Fix show_incremental_sort_info with force_parallel_mode

  10. Implement Incremental Sort

  11. Fix handling of "Subplans Removed" field in EXPLAIN output.

  12. Fix EXPLAIN (SETTINGS) to follow policy about when to print empty fields.

  13. Ensure plpgsql result tuples have the right composite type marking.

  14. Propagate sort instrumentation from workers back to leader.

  15. Make new regression test case parallel-safe, and improve its output.

  16. Push limit through subqueries to underlying sort, where possible.

  17. Fix inappropriate printing of never-measured times in EXPLAIN.

  18. Fix some infelicities in EXPLAIN output for parallel query plans.