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

On Tue, Mar 10, 2020 at 10:44 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
...
> Now, a couple comments about parts 0001 - 0003 of the patch ...
>
> 1) I see a bunch of failures in the regression test, due to minor
> differences in the explain output. All the differences are about minor
> changes in memory usage, like this:
>
> - "Sort Space Used": 30, +
> + "Sort Space Used": 29, +
>
> I'm not sure if it happens on my machine only, but maybe the test is not
> entirely stable.

make check passes on multiple machines for me; what arch/distro are you using?

Is there a better way to test these? I would prefer these code paths
have test coverage, but the standard SQL tests don't leave a good way
to handle stuff like this.

Is TAP the only alternative, and do you think it'd be worth considering?

> 2) I think this bit in ExecReScanIncrementalSort is wrong:
>
> node->sort_Done = false;
> tuplesort_end(node->fullsort_state);
> node->prefixsort_state = NULL;
> tuplesort_end(node->fullsort_state);
> node->prefixsort_state = NULL;
> node->bound_Done = 0;
>
> Notice both places reset fullsort_state and set prefixsort_state to
> NULL. Another thing is that I'm not sure it's fine to pass NULL to
> tuplesort_end (my guess is tuplesort_free will fail when it gets NULL).

Fixed.

James