Re: [HACKERS] [PATCH] Incremental sort - Mailing list pgsql-hackers
From | Alexander Korotkov |
---|---|
Subject | Re: [HACKERS] [PATCH] Incremental sort |
Date | |
Msg-id | CAPpHfdtZ3M8tw7vBPnmrc9X4LS65hEv_DrHJc_NoOB7erT2sqg@mail.gmail.com Whole thread Raw |
In response to | Re: [HACKERS] [PATCH] Incremental sort ("Tels" <nospam-pg-abuse@bloodgate.com>) |
List | pgsql-hackers |
Hi!
On Fri, Jan 5, 2018 at 2:21 AM, Tels <nospam-pg-abuse@bloodgate.com> wrote:
BTW, I also fixed documentation markup (regarding migration to xml).
Rebased patch is attached.
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);
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.
Rebased patch is attached.
The Russian Postgres Company
Attachment
pgsql-hackers by date: