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

On Tue, Mar 31, 2020 at 12:31 PM Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
>
> On 2020-Mar-30, James Coleman wrote:
>
> > +/* ----------------
> > + * Instruementation information for IncrementalSort
> > + * ----------------
> > + */
> > +typedef struct IncrementalSortGroupInfo
> > +{
> > + int64 groupCount;
> > + long maxDiskSpaceUsed;
> > + long totalDiskSpaceUsed;
> > + long maxMemorySpaceUsed;
> > + long totalMemorySpaceUsed;
> > + Size sortMethods; /* bitmask of TuplesortMethod */
> > +} IncrementalSortGroupInfo;
>
> There's a typo "Instruementation" in the comment, but I'm more surprised
> that type Size is being used to store a bitmask. It looks weird to me.
> Wouldn't it be more reasonable to use bits32 or some such? (I first
> noticed this in the "sizeof(Size)" code that appears in the explain
> code.)

I just didn't know about bits32; I'll change.

> OTOH, aesthetically it would seem to be better to define these values
> using ones and increasing shifts (1 << 1 and so on), rather than powers
> of two:
>
> > + * TuplesortMethod is used in a bitmask in Increment Sort's shared memory
> > + * instrumentation so needs to have each value be a separate bit.
> > */
> > typedef enum
> > {
> > SORT_TYPE_STILL_IN_PROGRESS = 0,
> > - SORT_TYPE_TOP_N_HEAPSORT,
> > - SORT_TYPE_QUICKSORT,
> > - SORT_TYPE_EXTERNAL_SORT,
> > - SORT_TYPE_EXTERNAL_MERGE
> > + SORT_TYPE_TOP_N_HEAPSORT = 2,
> > + SORT_TYPE_QUICKSORT = 4,
> > + SORT_TYPE_EXTERNAL_SORT = 8,
> > + SORT_TYPE_EXTERNAL_MERGE = 16
> > } TuplesortMethod;
>
> I don't quite understand why you skipped "1". (Also, is the use of zero
> a wise choice?)

The assignment of 0 was already there, and there wasn't a comment to
indicate why. That ends up meaning we wouldn't display "still in
progress" as a type here, which is maybe desirable, but I'm honestly
not sure why it was that way originally. I'm curious if you have any
thoughts on it.

I knew some projects used increasing shifts, but I wasn't sure what
the preference was here. I'll correct.

James