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

On Mon, Apr 06, 2020 at 05:47:48PM -0400, James Coleman wrote:
>On Mon, Apr 6, 2020 at 5:40 PM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>>
>> On Mon, Apr 06, 2020 at 11:12:32PM +0200, Tomas Vondra wrote:
>> >On Mon, Apr 06, 2020 at 04:54:38PM -0400, Alvaro Herrera wrote:
>> >>On 2020-Apr-06, Tom Lane wrote:
>> >>
>> >>>Locally, things pass without force_parallel_mode, but turning it on
>> >>>produces failures that look similar to rhinoceros's (didn't examine
>> >>>other BF members).
>> >>
>> >>FWIW I looked at the eight failures there were about fifteen minutes ago
>> >>and they were all identical.  I can confirm that, in my laptop, the
>> >>tests work without that GUC, and fail in exactly that way with it.
>> >>
>> >
>> >Yes, there's a thinko in show_incremental_sort_info() and it returns too
>> >soon. I'll push a fix in a minute.
>> >
>>
>> OK, I've pushed a fix - this should make the buildfarm happy again.
>>
>> It however seems to me a bit more needs to be done. The fix makes
>> show_incremental_sort_info closer to show_sort_info, but not entirely
>> because IncrementalSortState does not have sort_Done flag so it still
>> depends on (fullsortGroupInfo->groupCount > 0). I haven't noticed that
>> before, but not having that flag seems a bit weird to me.
>>
>> It also seems possibly incorrect - we may end up with
>>
>>    fullsortGroupInfo->groupCount == 0
>>    prefixsortGroupInfo->groupCount > 0
>>
>> but we won't print anything.
>
>This shouldn't ever be possible, because the only way we get any
>prefix groups at all is if we've already sorted a full sort group
>during the mode transition.
>
>> James, any opinion on this? I'd say we should restore the sort_Done flag
>> and make it work as in plain Sort. Or some comment explaining why
>> depending on the counts is OK (assuming it is).
>
>There's previous email traffic on this thread about that (I can look
>it up later this evening), but the short of it is that I believe that
>relying on the group count is actually more correct than a sort_Done
>flag in the case of incremental sort (in contrast to regular sort).
>

OK. Maybe we should add a comment to explain.c saying it's OK.

I've pushed a fix for failures due to different planned workers (in the
test I added to show changes due to add_partial_path tweaks).

It seems we're not out of the woods yet, though. rhinoceros and
sidewinder failed with something like this:

                 Sort Method: quicksort  Memory: NNkB
+               Sort Method: unknown  Disk: NNkB

Would you mind investigating at it?

regards

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