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

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.

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).

regards

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