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

On Fri, Jun 19, 2020 at 12:04 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> On Tue, Apr 07, 2020 at 08:40:30AM -0400, James Coleman wrote:
> > On Tue, Apr 7, 2020 at 12:25 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > > On Mon, Apr 06, 2020 at 09:57:22PM +0200, Tomas Vondra wrote:
> > > > I've pushed the fist part of this patch series - I've reorganized it a
> > >
> > > I scanned through this again post-commit. Find attached some suggestions.
> > >
> > > Shouldn't non-text explain output always show both disk *and* mem, including
> > > zeros ?
> >
> > Could you give more context on this? Is there a standard to follow?
> > Regular sort nodes only ever report one type, so there's not a good
> > parallel there.
>
> The change I proposed was like:
>
> @@ -2829,7 +2829,6 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
>
> ExplainPropertyList("Sort Methods Used", methodNames, es);
>
> - if (groupInfo->maxMemorySpaceUsed > 0)
> {
> long avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount;
> const char *spaceTypeName;
> ...
> - if (groupInfo->maxDiskSpaceUsed > 0)
> {
> ...
>
> To show in non-text format *both* disk and memory space used, even if zero.
>
> I still think that's what's desirable.

I'm of the opposite opinion. I believe showing both unnecessarily is confusing.

> If it's important to show *whether* a sort space was used, then I think there
> should be a boolean, or an int 0/1. But I don't think it's actually needed,
> since someone parsing the explain output could just check
> |if _dict['Peak Sort Space Used'] > 0: ...
> the same as you're doing, without having to write some variation on:
> |if 'Peak Sort Space Used' in _dict and _dict['Peak Sort Space Used'] > 0: ...

I think it's desirable for code to be explicitly about the type having
been used rather than implicitly assuming it based on 0/non-zero
values.

James