On Tue, Apr 07, 2020 at 10:53:05AM -0500, Justin Pryzby wrote:
> On Tue, Apr 07, 2020 at 08:40:30AM -0400, James Coleman wrote:
> > > And, should it use two spaces before "Sort Method", "Memory" and
> > > "Pre-sorted
> ...
> > I read through that subthread, and the ending seemed to be Peter
> > wanting things to be unified. Was there a conclusion beyond that?
>
> This discussion is ongoing. I think let's wait until that's settled before
> addressing this more complex and even newer case. We can add "explain, two
> spaces and equals vs colon" to the "Open items" list if need be - I hope the
> discussion will not delay the release.
The change proposed on the WAL thread is minimal, and makes new explain(WAL)
output consistent with the that of explain(BUFFERS).
That uses a different format from "Sort", which is what incremental sort should
follow. (Hashjoin also uses the Sort's format of two-spaces and colons rather
than equals).
So the attached 0001 makes explain output for incremental sort more consistent
with sort:
- Two spaces;
- colons rather than equals;
- Don't use semicolon, which isn't in use anywhere else;
I tested with this:
template1=# DROP TABLE t; CREATE TABLE t(i int, j int); INSERT INTO t SELECT
a-(a%100), a%1000 FROM generate_series(1,99999)a; CREATE INDEX ON t(i); VACUUM
VERBOSE ANALYZE t;
template1=# explain analyze SELECT * FROM t a ORDER BY i,j;
...
Full-sort Groups: 1000 Sort Method: quicksort Average Memory: 28kB Peak
Memory: 28kB Pre-sorted Groups: 1000 Sort Method: quicksort Average Memory:
30kB Peak Memory: 30kB
On Tue, Apr 07, 2020 at 05:34:15PM +0200, Tomas Vondra wrote:
> On Tue, Apr 07, 2020 at 08:40:30AM -0400, James Coleman wrote:
> > On Tue, Apr 7, 2020 at 12:25 AM Justin Pryzby <[email protected]> wrote:
> > > 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
> >
> > I'd originally had that, but Tomas wanted it to be more compact. It's
> > easy to adjust though if the consensus changes on that.
>
> I'm OK with changing the format if there's a consensus. The current
> format seemed better to me, but I'm not particularly attached to it.
I still think Pre-sorted groups should be on a separate line, as in 0002.
In addition to looking better (to me), and being easier to read, another reason
is that there are essentially key=>values here, but the keys are repeated (Sort
Method, etc).
I also suggested to rename: s/Presorted/Pre-sorted/, but I didn't do that here.
Michael already patched most of the comment typos, the remainder I'm including
here as a "nearby patch"..
--
Justin
>From 55044341f82b847d136cd17df5a3c8d80c8371b4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Wed, 15 Apr 2020 08:45:21 -0500
Subject: [PATCH v1 1/3] Fix explain output for incr sort:
- Two spaces;
- colons rather than equals;
- Don't use semicolon;
---
src/backend/commands/explain.c | 18 +++++++-----------
src/test/regress/expected/incremental_sort.out | 12 ++++++------
2 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 7ae6131676..9257c52707 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2778,7 +2778,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
{
if (indent)
appendStringInfoSpaces(es->str, es->indent * 2);
- appendStringInfo(es->str, "%s Groups: " INT64_FORMAT " Sort Method", groupLabel,
+ appendStringInfo(es->str, "%s Groups: " INT64_FORMAT " Sort Method", groupLabel,
groupInfo->groupCount);
/* plural/singular based on methodNames size */
if (list_length(methodNames) > 1)
@@ -2798,24 +2798,20 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
const char *spaceTypeName;
spaceTypeName = tuplesort_space_type_name(SORT_SPACE_TYPE_MEMORY);
- appendStringInfo(es->str, " %s: avg=%ldkB peak=%ldkB",
+ appendStringInfo(es->str, " Average %s: %ldkB Peak %s: %ldkB",
spaceTypeName, avgSpace,
- groupInfo->maxMemorySpaceUsed);
+ spaceTypeName, groupInfo->maxMemorySpaceUsed);
}
if (groupInfo->maxDiskSpaceUsed > 0)
{
long avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount;
-
const char *spaceTypeName;
spaceTypeName = tuplesort_space_type_name(SORT_SPACE_TYPE_DISK);
- /* Add a semicolon separator only if memory stats were printed. */
- if (groupInfo->maxMemorySpaceUsed > 0)
- appendStringInfo(es->str, ";");
- appendStringInfo(es->str, " %s: avg=%ldkB peak=%ldkB",
+ appendStringInfo(es->str, " Average %s: %ldkB Peak %s: %ldkB",
spaceTypeName, avgSpace,
- groupInfo->maxDiskSpaceUsed);
+ spaceTypeName, groupInfo->maxDiskSpaceUsed);
}
}
else
@@ -2899,7 +2895,7 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
if (prefixsortGroupInfo->groupCount > 0)
{
if (es->format == EXPLAIN_FORMAT_TEXT)
- appendStringInfo(es->str, " ");
+ appendStringInfo(es->str, " ");
show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", false, es);
}
if (es->format == EXPLAIN_FORMAT_TEXT)
@@ -2943,7 +2939,7 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
if (prefixsortGroupInfo->groupCount > 0)
{
if (es->format == EXPLAIN_FORMAT_TEXT)
- appendStringInfo(es->str, " ");
+ appendStringInfo(es->str, " ");
show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", false, es);
}
if (es->format == EXPLAIN_FORMAT_TEXT)
diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index 238d89a206..cf157a7aa1 100644
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -533,13 +533,13 @@ select * from (select * from t order by a) s order by a, b limit 55;
-- Test EXPLAIN ANALYZE with only a fullsort group.
select explain_analyze_without_memory('select * from (select * from t order by a) s order by a, b limit 55');
- explain_analyze_without_memory
-------------------------------------------------------------------------------------------------
+ explain_analyze_without_memory
+---------------------------------------------------------------------------------------------------------------
Limit (actual rows=55 loops=1)
-> Incremental Sort (actual rows=55 loops=1)
Sort Key: t.a, t.b
Presorted Key: t.a
- Full-sort Groups: 2 Sort Methods: top-N heapsort, quicksort Memory: avg=NNkB peak=NNkB
+ Full-sort Groups: 2 Sort Methods: top-N heapsort, quicksort Average Memory: NNkB Peak Memory: NNkB
-> Sort (actual rows=101 loops=1)
Sort Key: t.a
Sort Method: quicksort Memory: NNkB
@@ -708,13 +708,13 @@ select * from t left join (select * from (select * from t order by a) v order by
rollback;
-- Test EXPLAIN ANALYZE with both fullsort and presorted groups.
select explain_analyze_without_memory('select * from (select * from t order by a) s order by a, b limit 70');
- explain_analyze_without_memory
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ explain_analyze_without_memory
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Limit (actual rows=70 loops=1)
-> Incremental Sort (actual rows=70 loops=1)
Sort Key: t.a, t.b
Presorted Key: t.a
- Full-sort Groups: 1 Sort Method: quicksort Memory: avg=NNkB peak=NNkB Presorted Groups: 5 Sort Methods: top-N heapsort, quicksort Memory: avg=NNkB peak=NNkB
+ Full-sort Groups: 1 Sort Method: quicksort Average Memory: NNkB Peak Memory: NNkB Presorted Groups: 5 Sort Methods: top-N heapsort, quicksort Average Memory: NNkB Peak Memory: NNkB
-> Sort (actual rows=1000 loops=1)
Sort Key: t.a
Sort Method: quicksort Memory: NNkB
--
2.17.0
>From c8c4691e66d72c847d24ab547afa96f30fec1870 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Sat, 18 Apr 2020 21:02:59 -0500
Subject: [PATCH v1 2/3] Put Pre-sorted groups on a separate line
---
src/backend/commands/explain.c | 10 ++++++++--
src/test/regress/expected/incremental_sort.out | 9 +++++----
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 9257c52707..2ec5d5b810 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2895,7 +2895,10 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
if (prefixsortGroupInfo->groupCount > 0)
{
if (es->format == EXPLAIN_FORMAT_TEXT)
- appendStringInfo(es->str, " ");
+ {
+ appendStringInfo(es->str, "\n");
+ ExplainIndentText(es);
+ }
show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", false, es);
}
if (es->format == EXPLAIN_FORMAT_TEXT)
@@ -2939,7 +2942,10 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
if (prefixsortGroupInfo->groupCount > 0)
{
if (es->format == EXPLAIN_FORMAT_TEXT)
- appendStringInfo(es->str, " ");
+ {
+ appendStringInfo(es->str, "\n");
+ ExplainIndentText(es);
+ }
show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", false, es);
}
if (es->format == EXPLAIN_FORMAT_TEXT)
diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index cf157a7aa1..3460d2bd6f 100644
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -708,18 +708,19 @@ select * from t left join (select * from (select * from t order by a) v order by
rollback;
-- Test EXPLAIN ANALYZE with both fullsort and presorted groups.
select explain_analyze_without_memory('select * from (select * from t order by a) s order by a, b limit 70');
- explain_analyze_without_memory
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ explain_analyze_without_memory
+---------------------------------------------------------------------------------------------------------------
Limit (actual rows=70 loops=1)
-> Incremental Sort (actual rows=70 loops=1)
Sort Key: t.a, t.b
Presorted Key: t.a
- Full-sort Groups: 1 Sort Method: quicksort Average Memory: NNkB Peak Memory: NNkB Presorted Groups: 5 Sort Methods: top-N heapsort, quicksort Average Memory: NNkB Peak Memory: NNkB
+ Full-sort Groups: 1 Sort Method: quicksort Average Memory: NNkB Peak Memory: NNkB
+ Presorted Groups: 5 Sort Methods: top-N heapsort, quicksort Average Memory: NNkB Peak Memory: NNkB
-> Sort (actual rows=1000 loops=1)
Sort Key: t.a
Sort Method: quicksort Memory: NNkB
-> Seq Scan on t (actual rows=1000 loops=1)
-(9 rows)
+(10 rows)
select jsonb_pretty(explain_analyze_inc_sort_nodes_without_memory('select * from (select * from t order by a) s order by a, b limit 70'));
jsonb_pretty
--
2.17.0
>From f18a0d1f28c0ab8b9cd7e33ce7445830faa6e20d Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Mon, 6 Apr 2020 17:37:31 -0500
Subject: [PATCH v1 3/3] comment typos: Incremental Sort
commit d2d8a229bc58a2014dce1c7a4fcdb6c5ab9fb8da
Author: Tomas Vondra <[email protected]>
Previously reported here:
https://www.postgresql.org/message-id/20200407042521.GH2228%40telsasoft.com
---
src/backend/commands/explain.c | 4 ++--
src/backend/executor/nodeIncrementalSort.c | 8 +++++---
src/backend/utils/sort/tuplesort.c | 8 ++++----
3 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 2ec5d5b810..466666635b 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2865,7 +2865,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,
@@ -2916,7 +2916,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.
*/
diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c
index 39ba11cdf7..da99453c91 100644
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -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 |
@@ -1153,8 +1153,10 @@ 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
- * state similarly, doing so might actually cause a leak.
+ * repay the setup cost, and because ExecIncrementalSort guards
+ * presorted column functions by checking to see if the full sort state
+ * has been initialized yet, setting the sort states to null here 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 de38c6c7e0..c25a22f79b 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -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 fit 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 the amount of space occupied by some tupleset on the disk might
+ * be less than amount of space occupied by the same tupleset in
* memory due to more compact representation.
*/
if ((isSpaceDisk && !state->isMaxSpaceDisk) ||
--
2.17.0
