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 ?
Should "Pre-sorted Groups:" be on a separate line ?
| Full-sort Groups: 1 Sort Method: quicksort Memory: avg=28kB peak=28kB
Pre-sorted Groups: 1 Sort Method: quicksort Memory: avg=30kB peak=30kB
And, should it use two spaces before "Sort Method", "Memory" and "Pre-sorted
Groups"? I think you should maybe do that instead of the "semicolon
separator". I think "two spaces" makes sense, since the units are different,
similar to hash buckets and normal sort node.
"Buckets: %d Batches: %d Memory Usage: %ldkB\n",
appendStringInfo(es->str, "Sort Method: %s %s: %ldkB\n",
Note, I made a similar comment regarding two spaces for explain(WAL) here:
https://www.postgresql.org/message-id/20200402054120.GC14618%40telsasoft.com
And Peter E seemed to dislike that, here:
https://www.postgresql.org/message-id/ef8c966f-e50a-c583-7b1e-85de6f4ca0d3%402ndquadrant.com
Also, you're showing:
ExplainPropertyInteger("Maximum Sort Space Used", "kB",
groupInfo->maxMemorySpaceUsed, es);
But in show_hash_info() and show_hashagg_info(), and in your own text output,
that's called "Peak":
ExplainPropertyInteger("Peak Memory Usage", "kB", memPeakKb, es);
ExplainPropertyInteger("Peak Memory Usage", "kB",
spacePeakKb, es);
--
Justin
>From e26f2cc842792fc3c0dd4b4b97c0996d450c6dd7 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Mon, 6 Apr 2020 17:37:31 -0500
Subject: [PATCH v1] comment typos and others: Incremental Sort
commit d2d8a229bc58a2014dce1c7a4fcdb6c5ab9fb8da
Author: Tomas Vondra <[email protected]>
---
doc/src/sgml/perform.sgml | 2 +-
src/backend/commands/explain.c | 17 +++++++-------
src/backend/executor/nodeIncrementalSort.c | 26 +++++++++++-----------
src/backend/utils/sort/tuplesort.c | 10 ++++-----
src/include/utils/tuplesort.h | 2 +-
5 files changed, 28 insertions(+), 29 deletions(-)
diff --git a/doc/src/sgml/perform.sgml b/doc/src/sgml/perform.sgml
index 0dfc3e80e2..f448abd073 100644
--- a/doc/src/sgml/perform.sgml
+++ b/doc/src/sgml/perform.sgml
@@ -311,7 +311,7 @@ EXPLAIN SELECT * FROM tenk1 ORDER BY unique1;
-> Seq Scan on tenk1 (cost=0.00..445.00 rows=10000 width=244)
</screen>
- If the a part of the plan guarantess an ordering on a prefix of the
+ If a part of the plan guarantees an ordering on a prefix of the
required sort keys, then the planner may instead decide to use an
<literal>incremental sort</literal> step:
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index baaa5817af..ca4fe3307d 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2532,7 +2532,7 @@ show_sort_group_keys(PlanState *planstate, const char *qlabel,
ExplainPropertyList(qlabel, result, es);
if (nPresortedKeys > 0)
- ExplainPropertyList("Presorted Key", resultPresorted, es);
+ ExplainPropertyList("Pre-sorted Key", resultPresorted, es);
}
/*
@@ -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;
@@ -2841,12 +2840,12 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
ExplainOpenGroup("Sort Space", memoryName.data, true, es);
ExplainPropertyInteger("Average Sort Space Used", "kB", avgSpace, es);
- ExplainPropertyInteger("Maximum Sort Space Used", "kB",
+ ExplainPropertyInteger("Peak Sort Space Used", "kB",
groupInfo->maxMemorySpaceUsed, es);
ExplainCloseGroup("Sort Spaces", memoryName.data, true, es);
}
- if (groupInfo->maxDiskSpaceUsed > 0)
+
{
long avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
const char *spaceTypeName;
@@ -2858,7 +2857,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
ExplainOpenGroup("Sort Space", diskName.data, true, es);
ExplainPropertyInteger("Average Sort Space Used", "kB", avgSpace, es);
- ExplainPropertyInteger("Maximum Sort Space Used", "kB",
+ ExplainPropertyInteger("Peak Sort Space Used", "kB",
groupInfo->maxDiskSpaceUsed, es);
ExplainCloseGroup("Sort Spaces", diskName.data, true, es);
@@ -2869,7 +2868,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
}
/*
- * If it's EXPLAIN ANALYZE, show tuplesort stats for a incremental sort node
+ * If it's EXPLAIN ANALYZE, show tuplesort stats for an incremental sort node
*/
static void
show_incremental_sort_info(IncrementalSortState *incrsortstate,
@@ -2891,7 +2890,7 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
{
if (es->format == EXPLAIN_FORMAT_TEXT)
appendStringInfo(es->str, " ");
- show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", false, es);
+ show_incremental_sort_group_info(prefixsortGroupInfo, "Pre-sorted", false, es);
}
if (es->format == EXPLAIN_FORMAT_TEXT)
appendStringInfo(es->str, "\n");
@@ -2908,7 +2907,7 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
&incrsortstate->shared_info->sinfo[n];
/*
- * If a worker hasn't process any sort groups at all, then exclude
+ * If a worker hasn't processed any sort groups at all, then exclude
* it from output since it either didn't launch or didn't
* contribute anything meaningful.
*/
@@ -2928,7 +2927,7 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
{
if (es->format == EXPLAIN_FORMAT_TEXT)
appendStringInfo(es->str, " ");
- show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", false, es);
+ show_incremental_sort_group_info(prefixsortGroupInfo, "Pre-sorted", false, es);
}
if (es->format == EXPLAIN_FORMAT_TEXT)
appendStringInfo(es->str, "\n");
diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c
index bcab7c054c..afdec2a0cd 100644
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -270,7 +270,7 @@ isCurrentGroup(IncrementalSortState *node, TupleTableSlot *pivot, TupleTableSlot
* verify they're all part of the same prefix key group before sorting them
* solely by unsorted suffix keys.
*
- * While it's likely that all already fetch tuples are all part of a single
+ * While it's likely that all already fetched tuples are all part of a single
* prefix group, we also have to handle the possibility that there is at least
* one different prefix key group before the large prefix key group.
* ----------------------------------------------------------------
@@ -381,7 +381,7 @@ switchToPresortedPrefixMode(PlanState *pstate)
* node->transfer_tuple slot, and, even though that slot
* points to memory inside the full sort tuplesort, we can't
* reset that tuplesort anyway until we've fully transferred
- * out of its tuples, so this reference is safe. We do need to
+ * out its tuples, so this reference is safe. We do need to
* reset the group pivot tuple though since we've finished the
* current prefix key group.
*/
@@ -603,7 +603,7 @@ ExecIncrementalSort(PlanState *pstate)
/*
* Initialize presorted column support structures for
* isCurrentGroup(). It's correct to do this along with the
- * initial intialization for the full sort state (and not for the
+ * initial initialization for the full sort state (and not for the
* prefix sort state) since we always load the full sort state
* first.
*/
@@ -723,7 +723,7 @@ ExecIncrementalSort(PlanState *pstate)
nTuples++;
/*
- * If we've reach our minimum group size, then we need to
+ * If we've reached our minimum group size, then we need to
* store the most recent tuple as a pivot.
*/
if (nTuples == minGroupSize)
@@ -752,7 +752,7 @@ ExecIncrementalSort(PlanState *pstate)
{
/*
* Since the tuple we fetched isn't part of the current
- * prefix key group we don't want to sort it as part of
+ * prefix key group we don't want to sort it as part of
* the current batch. Instead we use the group_pivot slot
* to carry it over to the next batch (even though we
* won't actually treat it as a group pivot).
@@ -792,12 +792,12 @@ ExecIncrementalSort(PlanState *pstate)
}
/*
- * Unless we've alrady transitioned modes to reading from the full
+ * Unless we've already transitioned modes to reading from the full
* sort state, then we assume that having read at least
* DEFAULT_MAX_FULL_SORT_GROUP_SIZE tuples means it's likely we're
* processing a large group of tuples all having equal prefix keys
* (but haven't yet found the final tuple in that prefix key
- * group), so we need to transition in to presorted prefix mode.
+ * group), so we need to transition into presorted prefix mode.
*/
if (nTuples > DEFAULT_MAX_FULL_SORT_GROUP_SIZE &&
node->execution_status != INCSORT_READFULLSORT)
@@ -849,7 +849,7 @@ ExecIncrementalSort(PlanState *pstate)
/*
* We might have multiple prefix key groups in the full sort
- * state, so the mode transition function needs to know the it
+ * state, so the mode transition function needs to know that it
* needs to move from the fullsort to presorted prefix sort.
*/
node->n_fullsort_remaining = nTuples;
@@ -913,7 +913,7 @@ ExecIncrementalSort(PlanState *pstate)
/*
* If the tuple's prefix keys match our pivot tuple, we're not
* done yet and can load it into the prefix sort state. If not, we
- * don't want to sort it as part of the current batch. Instead we
+ * don't want to sort it as part of the current batch. Instead we
* use the group_pivot slot to carry it over to the next batch
* (even though we won't actually treat it as a group pivot).
*/
@@ -987,7 +987,7 @@ ExecInitIncrementalSort(IncrementalSort *node, EState *estate, int eflags)
/*
* Incremental sort can't be used with either EXEC_FLAG_REWIND,
- * EXEC_FLAG_BACKWARD or EXEC_FLAG_MARK, because we only one of many sort
+ * EXEC_FLAG_BACKWARD or EXEC_FLAG_MARK, because we only ???? one of many sort
* batches in the current sort state.
*/
Assert((eflags & (EXEC_FLAG_BACKWARD |
@@ -1121,14 +1121,14 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
PlanState *outerPlan = outerPlanState(node);
/*
- * Incremental sort doesn't support efficient rescan even when paramters
+ * Incremental sort doesn't support efficient rescan even when parameters
* haven't changed (e.g., rewind) because unlike regular sort we don't
* store all tuples at once for the full sort.
*
* So even if EXEC_FLAG_REWIND is set we just reset all of our state and
* reexecute the sort along with the child node below us.
*
- * In theory if we've only fill the full sort with one batch (and haven't
+ * In theory if we've only filled the full sort with one batch (and haven't
* reset it for a new batch yet) then we could efficiently rewind, but
* that seems a narrow enough case that it's not worth handling specially
* at this time.
@@ -1153,7 +1153,7 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
/*
* If we've set up either of the sort states yet, we need to reset them.
* We could end them and null out the pointers, but there's no reason to
- * repay the setup cost, and because guard setting up pivot comparator
+ * repay the setup cost, and because ???? guard setting up pivot comparator
* state similarly, doing so might actually cause a leak.
*/
if (node->fullsort_state != NULL)
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index cc33a85731..a965fb0025 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -808,7 +808,7 @@ tuplesort_begin_common(int workMem, SortCoordinate coordinate,
*
* Setup, or reset, all state need for processing a new set of tuples with this
* sort state. Called both from tuplesort_begin_common (the first time sorting
- * with this sort state) and tuplesort_reseti (for subsequent usages).
+ * with this sort state) and tuplesort_reset (for subsequent usages).
*/
static void
tuplesort_begin_batch(Tuplesortstate *state)
@@ -1428,11 +1428,11 @@ tuplesort_updatemax(Tuplesortstate *state)
}
/*
- * Sort evicts data to the disk when it didn't manage to fit those data to
- * the main memory. This is why we assume space used on the disk to be
+ * Sort evicts data to the disk when it didn't manage to fit the data in
+ * main memory. This is why we assume space used on the disk to be
* more important for tracking resource usage than space used in memory.
- * Note that amount of space occupied by some tuple set on the disk might
- * be less than amount of space occupied by the same tuple set in the
+ * Note that amount of space occupied by some tupleset on the disk might
+ * be less than amount of space occupied by the same tupleset in the
* memory due to more compact representation.
*/
if ((isSpaceDisk && !state->isMaxSpaceDisk) ||
diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h
index 04d263228d..d992b4875a 100644
--- a/src/include/utils/tuplesort.h
+++ b/src/include/utils/tuplesort.h
@@ -63,7 +63,7 @@ typedef struct SortCoordinateData *SortCoordinate;
* sometimes put it in shared memory.
*
* The parallel-sort infrastructure relies on having a zero TuplesortMethod
- * indicate that a worker never did anything, so we assign zero to
+ * to indicate that a worker never did anything, so we assign zero to
* SORT_TYPE_STILL_IN_PROGRESS. The other values of this enum can be
* OR'ed together to represent a situation where different workers used
* different methods, so we need a separate bit for each one. Keep the
--
2.17.0
