On Tue, Mar 10, 2020 at 10:44 PM Tomas Vondra
<tomas.vondra@2ndquadrant.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
