Date: Thu, 4 Jul 2019 16:46:11 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: James Coleman <jtc331@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>, Simon Riggs <simon@2ndquadrant.com>,
	Rafia Sabih <rafia.pghackers@gmail.com>,
	Shaun Thomas <shaun.thomas@2ndquadrant.com>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Message-ID: <20190704144611.gsichab2lbxnxaua@development>
References: 
 <CAAaqYe9pzNZnF328bFZfzR8zJK9Mvft3i6TN0NjxjCkWeVm1iQ@mail.gmail.com>
 <20190625160235.6ctryjzvw5je7cet@development>
 <CAAaqYe_JMQh0FeSSynr_j66ZNNiX3BjKoLiMgMF4zna1m0tcVg@mail.gmail.com>
 <CAH2-Wz=ptCW_dnRi3z4n8eUCzc82Y2qsd9ReWLr0-nxfhh=Hsg@mail.gmail.com>
 <CAAaqYe_pPWgcgjM5OVkwwgDPn9e63ZjFbv6ychiuHF_22B-FjQ@mail.gmail.com>
 <CAH2-Wz=L8dZNttnvSXvH6NTMWw2T5VsBBC4oU4tLTMV9CoLRvg@mail.gmail.com>
 <20190625203207.lvmmat7d2ilnr64t@development>
 <CAAaqYe-i7D+D+oih1AZ5QyyAMSWKjzNT7+eBz-RMr9i18zG96g@mail.gmail.com>
 <20190625232205.ssecrwplyb7faxbb@development>
 <CAAaqYe8PVSO3dzysZZ-RB8HRMAZ_JMOPwW--veY9ZEuTCcbe8A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAAaqYe8PVSO3dzysZZ-RB8HRMAZ_JMOPwW--veY9ZEuTCcbe8A@mail.gmail.com>
User-Agent: NeoMutt/20180716-1444-295967
Precedence: bulk

On Thu, Jul 04, 2019 at 09:29:49AM -0400, James Coleman wrote:
>On Tue, Jun 25, 2019 at 7:22 PM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>>
>> On Tue, Jun 25, 2019 at 04:53:40PM -0400, James Coleman wrote:
>> >
>> >Unrelated: if you or someone else you know that's more familiar with
>> >the parallel code, I'd be interested in their looking at the patch at
>> >some point, because I have a suspicion it might not be operating in
>...
>> So I've looked into that, and the reason seems fairly simple - when
>> generating the Gather Merge paths, we only look at paths that are in
>> partial_pathlist. See generate_gather_paths().
>>
>> And we only have sequential + index paths in partial_pathlist, not
>> incremental sort paths.
>>
>> IMHO we can do two things:
>>
>> 1) modify generate_gather_paths to also consider incremental sort for
>> each sorted path, similarly to what create_ordered_paths does
>>
>> 2) modify build_index_paths to also generate an incremental sort path
>> for each index path
>>
>> IMHO (1) is the right choice here, because it automatically does the
>> trick for all other types of ordered paths, not just index scans. So,
>> something like the attached patch, which gives me plans like this:
>...
>> But I'm not going to claim those are total fixes, it's the minimum I
>> needed to do to make this particular type of plan work.
>
>Thanks for looking into this!
>
>I intended to apply this to my most recent version of the patch (just
>sent a few minutes ago), but when I apply it I noticed that the
>partition_aggregate regression tests have several of these failures:
>
>ERROR:  could not find pathkey item to sort
>
>I haven't had time to look into the cause yet, so I decided to wait
>until the next patch revision.
>

FWIW I don't claim the patch I shared is complete and/or 100% correct.
It was more an illustration of the issue and the smallest patch to make
a particular query work. The test failures are a consequence of that.

I'll try looking into the failures over the next couple of days, but I
can't promise anything.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services