Re: [HACKERS] [PATCH] Incremental sort

Hi Alexander, On 06/01/2018 04:22 PM, Alexander Korotkov wrote: > Hi, James! > > On Thu, May 31, 2018 at 11:10 PM James Coleman > wrote: > > I've attached an updated copy of the patch that applies cleanly to > current master. > > > Thank you for rebasing this

Re: Re: [HACKERS] [PATCH] Incremental sort

Hi, James! On Thu, May 31, 2018 at 11:10 PM James Coleman wrote: > I've attached an updated copy of the patch that applies cleanly to > current master. > Thank you for rebasing this patch. Next time sending a patch, please make sure you've bumped its version, if even you made no changes there

Re: Re: [HACKERS] [PATCH] Incremental sort

I've attached an updated copy of the patch that applies cleanly to current master. commit 6428245702a40b3e3fa11bb64b7611cdd33a0778 Author: Alexander Korotkov Date: Sat Apr 7 18:51:20 2018 +0300 Implement incremental sort Incremental sort is an optimized variant of multikey sort

Re: [HACKERS] [PATCH] Incremental sort

On Tue, Apr 10, 2018 at 4:15 PM, David Steele wrote: > On 4/9/18 11:56 AM, Alexander Korotkov wrote: > > > > Thank you very much for your efforts on reviewing this patch. > > I'm looking forward work with you on this feature for PG12. > > Since there's a new patch I have changed the status to Nee

Re: [HACKERS] [PATCH] Incremental sort

On 4/9/18 11:56 AM, Alexander Korotkov wrote: > > Thank you very much for your efforts on reviewing this patch. > I'm looking forward work with you on this feature for PG12. Since there's a new patch I have changed the status to Needs Review and moved the entry to the next CF. Still, it seems to

Re: [HACKERS] [PATCH] Incremental sort

On Sat, Apr 7, 2018 at 11:57 PM, Tomas Vondra wrote: > On 04/07/2018 06:23 PM, Tom Lane wrote: > > Teodor Sigaev writes: > >>> I dunno, how would you estimate whether this is actually a win or not? > >>> I don't think our model of sort costs is anywhere near refined enough > >>> or accurate enou

Re: [HACKERS] [PATCH] Incremental sort

On 04/07/2018 06:23 PM, Tom Lane wrote: > Teodor Sigaev writes: >>> I dunno, how would you estimate whether this is actually a win or not? >>> I don't think our model of sort costs is anywhere near refined enough >>> or accurate enough to reliably predict whether this is better than >>> just doing

Re: [HACKERS] [PATCH] Incremental sort

Teodor Sigaev writes: >> I dunno, how would you estimate whether this is actually a win or not? >> I don't think our model of sort costs is anywhere near refined enough >> or accurate enough to reliably predict whether this is better than >> just doing it in one step. Even if the cost model is go

Re: [HACKERS] [PATCH] Incremental sort

On Sat, Apr 7, 2018 at 5:37 PM, Teodor Sigaev wrote: > by this patch. Revised version is attached. >> > > Fine, patch got several rounds of review in all its parts. Is any places > which should be improved before commit? Also I found that after planner changes of Alexander Kuzmenkov, increment

Re: [HACKERS] [PATCH] Incremental sort

I dunno, how would you estimate whether this is actually a win or not? I don't think our model of sort costs is anywhere near refined enough or accurate enough to reliably predict whether this is better than just doing it in one step. Even if the cost model is good, it's not going to be better th

Re: [HACKERS] [PATCH] Incremental sort

On 2018-04-07 12:06:52 -0400, Tom Lane wrote: > Alexander Korotkov writes: > > On Wed, Mar 28, 2018 at 6:38 PM, Alvaro Herrera > > wrote: > >> Can we have a recap on what the patch *does*? > > > Ggeneral idea hasn't been changed much since first email. > > Incremental sort gives benefit when you

Re: [HACKERS] [PATCH] Incremental sort

Alexander Korotkov writes: > On Wed, Mar 28, 2018 at 6:38 PM, Alvaro Herrera > wrote: >> Can we have a recap on what the patch *does*? > Ggeneral idea hasn't been changed much since first email. > Incremental sort gives benefit when you need to sort your dataset > by some list of columns while y

Re: [HACKERS] [PATCH] Incremental sort

On Wed, Mar 28, 2018 at 6:38 PM, Alvaro Herrera wrote: > Teodor Sigaev wrote: > > > BTW, patch had conflicts with master. Please, find rebased version > attached. > > > > Despite by patch conflist patch looks commitable, has anybody objections > to > > commit it? > > > > Patch recieved several r

Re: [HACKERS] [PATCH] Incremental sort

On 04/07/2018 04:37 PM, Teodor Sigaev wrote: >> by this patch.  Revised version is attached. > > Fine, patch got several rounds of review in all its parts. Is any > places which should be improved before commit? > I personally feel rather uneasy about committing it, TBH. While I don't see any o

Re: [HACKERS] [PATCH] Incremental sort

by this patch.  Revised version is attached. Fine, patch got several rounds of review in all its parts. Is any places which should be improved before commit? -- Teodor Sigaev E-mail: teo...@sigaev.ru WWW: http://www.sigaev.ru/

Re: [HACKERS] [PATCH] Incremental sort

On Sat, Apr 7, 2018 at 4:56 PM, Alexander Korotkov < a.korot...@postgrespro.ru> wrote: > On Fri, Apr 6, 2018 at 11:40 PM, Alexander Kuzmenkov < > a.kuzmen...@postgrespro.ru> wrote: > >> On 06.04.2018 20:26, Tomas Vondra wrote: >> >>> I personally am OK with reducing the scope of the patch like thi

Re: [HACKERS] [PATCH] Incremental sort

On Fri, Apr 6, 2018 at 11:40 PM, Alexander Kuzmenkov < a.kuzmen...@postgrespro.ru> wrote: > On 06.04.2018 20:26, Tomas Vondra wrote: > >> I personally am OK with reducing the scope of the patch like this. It's >> still beneficial for the common ORDER BY + LIMIT case, which is good. I >> don't thin

Re: [HACKERS] [PATCH] Incremental sort

Hi all, This is the other Alexander K. speaking. On 06.04.2018 20:26, Tomas Vondra wrote: I personally am OK with reducing the scope of the patch like this. It's still beneficial for the common ORDER BY + LIMIT case, which is good. I don't think it may negatively affect other cases (at least I

Re: [HACKERS] [PATCH] Incremental sort

On 04/06/2018 01:43 AM, Alexander Korotkov wrote: > Hi! > > On Tue, Apr 3, 2018 at 2:10 PM, Tomas Vondra > mailto:tomas.von...@2ndquadrant.com>> wrote: > > I think solving this may be fairly straight-forward. Essentially, until > now we only had one way to do the sort, so it was OK to mak

Re: [HACKERS] [PATCH] Incremental sort

On 04/03/2018 11:09 AM, Alexander Korotkov wrote: > On Sun, Apr 1, 2018 at 12:06 AM, Tomas Vondra > mailto:tomas.von...@2ndquadrant.com>> wrote: > > On 03/31/2018 10:43 PM, Tomas Vondra wrote: > > ... > > But I'm pretty sure it may lead to surprising behavior - for example if > > y

Re: [HACKERS] [PATCH] Incremental sort

On Sun, Apr 1, 2018 at 12:06 AM, Tomas Vondra wrote: > On 03/31/2018 10:43 PM, Tomas Vondra wrote: > > ... > > But I'm pretty sure it may lead to surprising behavior - for example if > > you disable incremental sorts (enable_incrementalsort=off), the plan > > will switch to plain sort without the

Re: [HACKERS] [PATCH] Incremental sort

On 03/31/2018 10:43 PM, Tomas Vondra wrote: > ... > But I'm pretty sure it may lead to surprising behavior - for example if > you disable incremental sorts (enable_incrementalsort=off), the plan > will switch to plain sort without the additional costs. So you'll get a > cheaper plan by disabling s

Re: [HACKERS] [PATCH] Incremental sort

Hi, I've been doing a bit more review of the patch today, focusing on the planner part, and I'm starting to have some doubts regarding the way incremental sort paths are created. I do have some question about the executor and other parts too. I'll mark this as 'waiting on author' to make it clear

Re: [HACKERS] [PATCH] Incremental sort

Hi Alexander, I took a quick look at the patch. Some things I fixed myself in the attached patch v.21. Here is the summary: Typo in compare_fractional_path_costs() should be fixed as a separate patch. Remove unused function estimate_pathkeys_groups. Extra MemoryContextReset before tuplesort_en

Re: [HACKERS] [PATCH] Incremental sort

On Wed, Mar 28, 2018 at 7:17 PM, Andres Freund wrote: > On 2018-03-28 16:28:01 +0300, Teodor Sigaev wrote: > > > BTW, patch had conflicts with master. Please, find rebased version > attached. > > > > Despite by patch conflist patch looks commitable, has anybody objections > to > > commit it? > >

Re: [HACKERS] [PATCH] Incremental sort

Hi, On 2018-03-28 16:28:01 +0300, Teodor Sigaev wrote: > > BTW, patch had conflicts with master.  Please, find rebased version > > attached. > > Despite by patch conflist patch looks commitable, has anybody objections to > commit it? > Patch recieved several rounds of review during 2 years, and

Re: [HACKERS] [PATCH] Incremental sort

Teodor Sigaev wrote: > > BTW, patch had conflicts with master.  Please, find rebased version > > attached. > > Despite by patch conflist patch looks commitable, has anybody objections to > commit it? > > Patch recieved several rounds of review during 2 years, and seems to me, > keeping it out fr

Re: [HACKERS] [PATCH] Incremental sort

On 03/28/2018 05:12 PM, Alexander Korotkov wrote: > On Wed, Mar 28, 2018 at 4:44 PM, Tomas Vondra > mailto:tomas.von...@2ndquadrant.com>> wrote: > > On 03/28/2018 03:28 PM, Teodor Sigaev wrote: > >> BTW, patch had conflicts with master.  Please, find rebased version > >> attached. >

Re: [HACKERS] [PATCH] Incremental sort

On 03/28/2018 03:28 PM, Teodor Sigaev wrote: >> BTW, patch had conflicts with master.  Please, find rebased version >> attached. > > Despite by patch conflist patch looks commitable, has anybody objections > to commit it? > > Patch recieved several rounds of review during 2 years, and seems to me

Re: [HACKERS] [PATCH] Incremental sort

BTW, patch had conflicts with master.  Please, find rebased version attached. Despite by patch conflist patch looks commitable, has anybody objections to commit it? Patch recieved several rounds of review during 2 years, and seems to me, keeping it out from sources may cause a lost it. Altho

Re: [HACKERS] [PATCH] Incremental sort

BTW, patch had conflicts with master.  Please, find rebased version attached. Sorry, but patch conflicts with master again. -- Teodor Sigaev E-mail: teo...@sigaev.ru WWW: http://www.sigaev.ru/

Re: [HACKERS] [PATCH] Incremental sort

Hi! On Wed, Mar 21, 2018 at 2:30 PM, Darafei "Komяpa" Praliaskouski < m...@komzpa.net> wrote: > on a PostGIS system tuned for preferring parallelism heavily ( > min_parallel_table_scan_size=10kB) we experience issues with QGIS table > discovery query with this patch: > > Failing query is: > [loca

Re: [HACKERS] [PATCH] Incremental sort

Hi, on a PostGIS system tuned for preferring parallelism heavily ( min_parallel_table_scan_size=10kB) we experience issues with QGIS table discovery query with this patch: Failing query is: [local] gis@gis=# SELECT l.f_table_name,l.f_table_schema,l.f_geometry_column,upper(l.type),l.srid,l.coord_d

Re: [HACKERS] [PATCH] Incremental sort

On 03/16/2018 09:47 AM, Alexander Korotkov wrote: > On Fri, Mar 16, 2018 at 5:12 AM, Tomas Vondra > mailto:tomas.von...@2ndquadrant.com>> wrote: > > I agree those don't seem like an issue in the Incremental Sort patch, > but like a more generic costing problems. > > > Yes, I think so too

Re: [HACKERS] [PATCH] Incremental sort

On Fri, Mar 16, 2018 at 5:12 AM, Tomas Vondra wrote: > I agree those don't seem like an issue in the Incremental Sort patch, > but like a more generic costing problems. > Yes, I think so too. Do you think we can mark this patch RFC assuming that it have already got pretty much of review previous

Re: [HACKERS] [PATCH] Incremental sort

On 03/10/2018 06:05 PM, Alexander Korotkov wrote: > On Sat, Mar 10, 2018 at 6:42 PM, Alexander Korotkov > mailto:a.korot...@postgrespro.ru>> wrote: > > ... > > After some investigation of benchmark results, I found 2 sources of > regressions of incremental sort. > > *Case 1: Underlying node scan

Re: [HACKERS] [PATCH] Incremental sort

On Sat, Mar 10, 2018 at 6:42 PM, Alexander Korotkov < a.korot...@postgrespro.ru> wrote: > On Thu, Mar 8, 2018 at 2:49 AM, Tomas Vondra > wrote: > Thank you very much for testing and benchmarking. I'll investigate > the regressions you found. > > >> Now, there's a caveat in those tests - the data

Re: [HACKERS] [PATCH] Incremental sort

On Thu, Mar 8, 2018 at 2:49 AM, Tomas Vondra wrote: > OK, the revised patch works fine - I've done a lot of testing and > benchmarking, and not a single segfault or any other crash. > > Regarding the benchmarks, I generally used queries of the form > > SELECT * FROM (SELECT * FROM t ORDER BY

Re: [HACKERS] [PATCH] Incremental sort

Hi, I have started reviewing the patch and doing some testing, and I have pretty quickly ran into a segfault. Attached is a simple reproducer and an backtrace. AFAICS the bug seems to be somewhere in the tuplesort changes, likely resetting a memory context too soon or something like that. I haven'

Re: [HACKERS] [PATCH] Incremental sort

On Mon, Jan 8, 2018 at 2:29 PM, Antonin Houska wrote: > Alexander Korotkov wrote: > > > Antonin Houska wrote: > > > > Shouldn't the test contain *both* cases? > > > Thank you for pointing that. Sure, both cases are better. I've added > second case as well as comments. Patch is attached. > > I'

Re: [HACKERS] [PATCH] Incremental sort

Alexander Korotkov wrote: > Antonin Houska wrote: > > Shouldn't the test contain *both* cases? > Thank you for pointing that. Sure, both cases are better. I've added second > case as well as comments. Patch is attached. I'm fine with the tests now but have a minor comment on this comment:

Re: [HACKERS] [PATCH] Incremental sort

Hello Alexander, On Thu, January 4, 2018 4:36 pm, Alexander Korotkov wrote: > On Fri, Dec 8, 2017 at 4:06 PM, Alexander Korotkov < > a.korot...@postgrespro.ru> wrote: > >> Thank you for pointing that. Sure, both cases are better. I've added >> second case as well as comments. Patch is attached.

Re: [HACKERS] [PATCH] Incremental sort

On Fri, Dec 8, 2017 at 4:06 PM, Alexander Korotkov < a.korot...@postgrespro.ru> wrote: > Thank you for pointing that. Sure, both cases are better. I've added > second case as well as comments. Patch is attached. > I just found that patch apply is failed according to commitfest.cputube.org. Ple

Re: [HACKERS] [PATCH] Incremental sort

Hi! On Fri, Dec 1, 2017 at 11:39 AM, Antonin Houska wrote: > I expected the number of groups actually that actually appear in the > output, > you consider it the number of groups started. I can't find similar case > elsewhere in the code (e.g. Agg node does not report this kind of > information)

Re: [HACKERS] [PATCH] Incremental sort

On Wed, Nov 22, 2017 at 12:01 AM, Thomas Munro < thomas.mu...@enterprisedb.com> wrote: > >> I gather that you have > >> determined empirically that it's better to be able to sort groups of > >> at least MIN_GROUP_SIZE than to be able to skip the comparisons on the > >> leading attributes, but why

Re: [HACKERS] [PATCH] Incremental sort

Alexander Korotkov wrote: > On Wed, Nov 22, 2017 at 1:22 PM, Antonin Houska wrote: > > Alexander Korotkov wrote: > > > Antonin Houska wrote: > > > > * ExecIncrementalSort() > > > > > > ** if (node->tuplesortstate == NULL) > > > > > > If both branches contain the expression > > > >

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

On Mon, Mar 20, 2017 at 6:33 PM, Alexander Korotkov wrote: > Thank you for the report. > Please, find rebased patch in the attachment. This patch cannot be applied. Please provide a rebased version. I am moving it to next CF with waiting on author as status. -- Michael

Re: [HACKERS] [PATCH] Incremental sort

Alexander Korotkov wrote: > Antonin Houska wrote: > > * ExecIncrementalSort() > > > > ** if (node->tuplesortstate == NULL) > > > > If both branches contain the expression > > > > node->groupsCount++; > > > > I suggest it to be moved outside the "if" construct. > > Done. One more comme

Re: [HACKERS] [PATCH] Incremental sort

On Tue, Nov 21, 2017 at 1:00 PM, Alexander Korotkov wrote: > On Mon, Nov 20, 2017 at 12:24 AM, Thomas Munro > wrote: >> Is there some reason not to use ApplySortComparator for this? I think >> you're missing out on lower-overhead comparators, and in any case it's >> probably good code reuse, no?

Re: [HACKERS] [PATCH] Incremental sort

On Mon, Nov 20, 2017 at 3:34 PM, Alexander Korotkov wrote: > Thank you very much for review. I really appreciate this topic gets > attention. Please, find next revision of patch in the attachment. I would really like to see this get into v11. This is an important patch, that has fallen through

Re: [HACKERS] [PATCH] Incremental sort

Hi! On Mon, Nov 20, 2017 at 12:24 AM, Thomas Munro < thomas.mu...@enterprisedb.com> wrote: > On Wed, Nov 15, 2017 at 7:42 AM, Alexander Korotkov > wrote: > > Sure, please find rebased patch attached. > > + /* > + * Check if first "skipCols" sort values are equal. > + */ > + static bool > + cmp

Re: [HACKERS] [PATCH] Incremental sort

On Wed, Nov 15, 2017 at 7:42 AM, Alexander Korotkov wrote: > Sure, please find rebased patch attached. + /* + * Check if first "skipCols" sort values are equal. + */ + static bool + cmpSortSkipCols(IncrementalSortState *node, TupleTableSlot *a, +

Re: [HACKERS] [PATCH] Incremental sort

Antonin Houska wrote: > Alexander Korotkov wrote: > > > Patch rebased to current master is attached. I'm going to improve my > > testing script and post new results. > > I wanted to review this patch but incremental-sort-8.patch fails to apply. Can > you please rebase it again? I could find

Re: [HACKERS] [PATCH] Incremental sort

Alexander Korotkov wrote: > Patch rebased to current master is attached. I'm going to improve my testing > script and post new results. I wanted to review this patch but incremental-sort-8.patch fails to apply. Can you please rebase it again? -- Antonin Houska Cybertec Schönig & Schönig GmbH