Re: [HACKERS] [PATCH] Incremental sort

Hi!

On Fri, Jan 5, 2018 at 2:21 AM, Tels <nospam-pg-abuse(at)bloodgate(dot)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(dot)korotkov(at)postgrespro(dot)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);
>

It's quite common practice to align second argument to the same position as
first argument. See other lines nearby.

> Or like this:
>
> + result = (PlanState *) ExecInitIncrementalSort(
> +
> (IncrementalSort *) node, estate, eflags);
>

It was probably not so good idea to insert line break before first
argument. Fixed.

>
> 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."
>

Many thanks for noticing these. Fixed.

> + "Portions Copyright (c) 1996-2017"
>
> Should probably be 2018 now - time flies fast :)
>

Right. Happy New Year! :)

> 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?
>

Good catch, thank you!

>
> + space,
> fase when it's value for in-memory
>
> typo: "space, false when ..."
>

Right. Fixed.

> + 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))
>

Right. This comes from time when there was more complicated code which
have to use the cmp variable multiple times.

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

Anyway, it's code simplification which is good regardless whether compilers
able to do it themselves or not.

+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.
>

I'd like to leave common members between of SortState and
IncrementalSortState to be ordered the same way.
Thus, I think that if we're going to reorder then we should do this in both
data structures.
But I'm not sure it worth considering, because these data structures are
very unlikely be the source of significant memory consumption...

That's all for now :)
>

Great, thank you for review.

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

Rebased patch is attached.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company