Re: [HACKERS] [PATCH] Incremental sort (was: PoC: Partial sort)
On Mon, Feb 27, 2017 at 8:29 PM, Alexander Korotkov wrote: This patch needs to be rebased. 1. It fails while applying as below patching file src/test/regress/expected/sysviews.out Hunk #1 FAILED at 70. 1 out of 1 hunk FAILED -- saving rejects to file src/test/regress/expected/sysviews.out.rej patching file src/test/regress/sql/inherit.sql 2. Also, there are compilation errors due to new commits. -fwrapv -fexcess-precision=standard -O2 -I../../../../src/include -D_GNU_SOURCE -c -o createplan.o createplan.c createplan.c: In function ‘create_gather_merge_plan’: createplan.c:1510:11: warning: passing argument 3 of ‘make_sort’ makes integer from pointer without a cast [enabled by default] gm_plan->nullsFirst); ^ createplan.c:235:14: note: expected ‘int’ but argument is of type ‘AttrNumber *’ static Sort *make_sort(Plan *lefttree, int numCols, int skipCols, ^ createplan.c:1510:11: warning: passing argument 4 of ‘make_sort’ from incompatible pointer type [enabled by default] gm_plan->nullsFirst); -- Thanks and Regards Mithun C Y EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Incremental sort (was: PoC: Partial sort)
On Sat, Feb 18, 2017 at 4:01 PM, Alexander Korotkov 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? 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. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Incremental sort (was: PoC: Partial sort)
Hi all! 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. Also patch has set of assorted code and comments improvements. Links 1. https://www.postgresql.org/message-id/CA+TgmoZapyHRm7NVyuyZ+yAV=u1a070bogre7pkgyraegr4...@mail.gmail.com 2. https://www.postgresql.org/message-id/cam3swzql4yd2sndhemcgl0q2b2otdkuvv_l6zg_fcgoluwm...@mail.gmail.com -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company incremental-sort-1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers