On Mon, Apr 6, 2020 at 6:13 PM Tomas Vondra <tomas.von...@2ndquadrant.com> wrote: > > 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.von...@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?
I assume that means those build farm members run with very low work_mem? Is it an acceptable fix to adjust work_mem up a bit just for these tests? Or is that bad practice and these are to expose issues with changing into disk sort mode? James
