Re: [HACKERS] [PATCH] Incremental sort

Hi!

On Fri, Jan 5, 2018 at 2:21 AM, Tels <nospam-pg-abuse@bloodgate.com> wrote:

On Thu, January 4, 2018 4:36 pm, Alexander Korotkov wrote:
> On Fri, Dec 8, 2017 at 4:06 PM, Alexander Korotkov <
> a.korotkov@postgrespro.ru> wrote:
>
>> Thank you for pointing that.  Sure, both cases are better.  I've added
>> second case as well as comments.  Patch is attached.

I had a quick look, this isn't a full review, but a few things struck me
on a read through the diff:

There are quite a few places where lines are broken like so:

+                       ExecIncrementalSortInitializeWorker((IncrementalSortState *) planstate,
+                                                                                               pwcxt);

Or like this:

+                       result = (PlanState *) ExecInitIncrementalSort(
+                                                                       (IncrementalSort *) node, estate, eflags);

e.g. a param is on the next line, but aligned to the very same place where
it would be w/o the linebreak. Or is this just some sort of artefact
because I viewed the diff with tabspacing = 8?

I'd fix the grammar here:

+ *             Incremental sort is specially optimized kind of multikey sort when
+ *             input is already presorted by prefix of required keys list.

Like so:

"Incremental sort is a specially optimized kind of multikey sort used when
the input is already presorted by a prefix of the required keys list."

+ *             Consider following example.  We have input tuples consisting from

"Consider the following example: We have ..."

+                * In incremental sort case we also have to cost to detect sort groups.

"we also have to cost the detection of sort groups."

"+               * It turns out into extra copy and comparison for each tuple."

"This turns out to be one extra copy and comparison per tuple."

+ "Portions Copyright (c) 1996-2017"

Should probably be 2018 now - time flies fast :)

                return_value = _readMaterial();
        else if (MATCH("SORT", 4))
                return_value = _readSort();
+       else if (MATCH("INCREMENTALSORT", 7))
+               return_value = _readIncrementalSort();
        else if (MATCH("GROUP", 5))
                return_value = _readGroup();

I think the ", 7" here is left-over from when it was named "INCSORT", and
it should be MATCH("INCREMENTALSORT", 15)), shouldn't it?

+                                                                  space, fase when it's value for in-memory

typo: "space, false when ..."

+                       bool    cmp;
+                       cmp = cmpSortSkipCols(node, node->sampleSlot, slot);
+
+                       if (cmp)

In the above, the variable cmp could be optimized away with:

+                       if (cmpSortSkipCols(node, node->sampleSlot, slot))

(not sure if modern compilers won't do this, anway, though)

+typedef struct IncrementalSortState
+{
+       ScanState       ss;                             /* its first field is NodeTag */
+       bool            bounded;                /* is the result set
bounded? */
+       int64           bound;                  /* if bounded, how many
tuples are needed */

If I'm not wrong, the layout of the struct will include quite a bit of
padding on 64 bit due to the mixing of bool and int64, maybe it would be
better to sort the fields differently, e.g. pack 4 or 8 bools together?
Not sure if that makes much of a difference, though.

That's all for now :)

BTW, I also fixed documentation markup (regarding migration to xml).

Rebased patch is attached.