Checking if it's possible to address this Opened Item before 13b1. https://wiki.postgresql.org/wiki/PostgreSQL_13_Open_Items consistency of explain output: two spaces, equals vs colons, semicolons (incremental sort)
On Sun, Apr 19, 2020 at 09:46:55AM -0400, James Coleman wrote: > On Sat, Apr 18, 2020 at 10:36 PM Justin Pryzby <pry...@telsasoft.com> wrote: > > > > 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). > > I think it's not great that buffers/sort are different, but I agree > that we should follow sort. > > > 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 <pry...@telsasoft.com> > > > > 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 collapsed this into 0001 because I think that if we're going to do > away with the key=value style then we effectively to have to do this > to avoid the repeated values being confusing (with key=value it kinda > worked, because that made it seem like the avg/peak were clearly a > subset of the Sort Groups info). > > I also cleaned up the newline patch a bit in the process (we already > have a way to indent with a flag so don't need to do it directly). > > > I also suggested to rename: s/Presorted/Pre-sorted/, but I didn't do that > > here. > > I went ahead and did that too; we already use "Full-sort", so the > proposed change makes both parallel. > > > Michael already patched most of the comment typos, the remainder I'm > > including > > here as a "nearby patch".. > > Modified slightly. > > James > From becd60ba348558fa241db5cc2100a84b757cbdc5 Mon Sep 17 00:00:00 2001 > From: Justin Pryzby <pryz...@telsasoft.com> > Date: Mon, 6 Apr 2020 17:37:31 -0500 > Subject: [PATCH v2 2/2] comment typos: Incremental Sort > > commit d2d8a229bc58a2014dce1c7a4fcdb6c5ab9fb8da > Author: Tomas Vondra <tomas.von...@postgresql.org> > > Previously reported here: > https://www.postgresql.org/message-id/20200407042521.GH2228%40telsasoft.com > --- > src/backend/commands/explain.c | 4 ++-- > src/backend/executor/nodeIncrementalSort.c | 10 ++++++---- > src/backend/utils/sort/tuplesort.c | 8 ++++---- > 3 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c > index 5f91c569a0..86c10458f4 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, > @@ -2913,7 +2913,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..05c60ec3e0 100644 > --- a/src/backend/executor/nodeIncrementalSort.c > +++ b/src/backend/executor/nodeIncrementalSort.c > @@ -987,8 +987,8 @@ 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 > - * batches in the current sort state. > + * EXEC_FLAG_BACKWARD or EXEC_FLAG_MARK, because the current sort state > + * contains only one sort batch rather than the full result set. > */ > Assert((eflags & (EXEC_FLAG_BACKWARD | > EXEC_FLAG_MARK)) == 0); > @@ -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..d59e3d5a8d 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 wasn't able to fit that data > into > + * 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.1 > > From 8e80be2b345c3940f76ffbd5e3c201a7ae855784 Mon Sep 17 00:00:00 2001 > From: Justin Pryzby <pryz...@telsasoft.com> > Date: Wed, 15 Apr 2020 08:45:21 -0500 > Subject: [PATCH v2 1/2] Fix explain output for incr sort: > > - Two spaces > - colons rather than equals > - Don't use semicolon > - Put Pre-sorted groups on a separate line > - Rename Presorted to Pre-sorted (to match Full-sort) > --- > src/backend/commands/explain.c | 22 ++++++++----------- > .../regress/expected/incremental_sort.out | 21 +++++++++--------- > src/test/regress/sql/incremental_sort.sql | 4 ++-- > 3 files changed, 22 insertions(+), 25 deletions(-) > > diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c > index 7ae6131676..5f91c569a0 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,8 +2895,8 @@ show_incremental_sort_info(IncrementalSortState > *incrsortstate, > if (prefixsortGroupInfo->groupCount > 0) > { > if (es->format == EXPLAIN_FORMAT_TEXT) > - appendStringInfo(es->str, " "); > - show_incremental_sort_group_info(prefixsortGroupInfo, > "Presorted", false, es); > + appendStringInfo(es->str, "\n"); > + show_incremental_sort_group_info(prefixsortGroupInfo, > "Pre-sorted", true, es); > } > if (es->format == EXPLAIN_FORMAT_TEXT) > appendStringInfo(es->str, "\n"); > @@ -2943,8 +2939,8 @@ show_incremental_sort_info(IncrementalSortState > *incrsortstate, > if (prefixsortGroupInfo->groupCount > 0) > { > if (es->format == EXPLAIN_FORMAT_TEXT) > - appendStringInfo(es->str, " "); > - > show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", false, es); > + appendStringInfo(es->str, "\n"); > + > show_incremental_sort_group_info(prefixsortGroupInfo, "Pre-sorted", true, es); > } > if (es->format == EXPLAIN_FORMAT_TEXT) > appendStringInfo(es->str, "\n"); > diff --git a/src/test/regress/expected/incremental_sort.out > b/src/test/regress/expected/incremental_sort.out > index 238d89a206..2b40a26d82 100644 > --- a/src/test/regress/expected/incremental_sort.out > +++ b/src/test/regress/expected/incremental_sort.out > @@ -106,7 +106,7 @@ declare > space_key text; > begin > for node in select * from > jsonb_array_elements(explain_analyze_inc_sort_nodes(query)) t loop > - for group_key in select unnest(array['Full-sort Groups', 'Presorted > Groups']::text[]) t loop > + for group_key in select unnest(array['Full-sort Groups', 'Pre-sorted > Groups']::text[]) t loop > for space_key in select unnest(array['Sort Space Memory', 'Sort Space > Disk']::text[]) t loop > node := jsonb_set(node, array[group_key, space_key, 'Average Sort > Space Used'], '"NN"', false); > node := jsonb_set(node, array[group_key, space_key, 'Peak Sort Space > Used'], '"NN"', false); > @@ -128,7 +128,7 @@ declare > space_key text; > begin > for node in select * from > jsonb_array_elements(explain_analyze_inc_sort_nodes(query)) t loop > - for group_key in select unnest(array['Full-sort Groups', 'Presorted > Groups']::text[]) t loop > + for group_key in select unnest(array['Full-sort Groups', 'Pre-sorted > Groups']::text[]) t loop > group_stats := node->group_key; > for space_key in select unnest(array['Sort Space Memory', 'Sort Space > Disk']::text[]) t loop > if (group_stats->space_key->'Peak Sort Space Used')::bigint < > (group_stats->space_key->'Peak Sort Space Used')::bigint then > @@ -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,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 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 > + Pre-sorted 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 > @@ -747,7 +748,7 @@ select > jsonb_pretty(explain_analyze_inc_sort_nodes_without_memory('select * from > "Average Sort Space Used": "NN"+ > } + > }, + > - "Presorted Groups": { + > + "Pre-sorted Groups": { + > "Group Count": 5, + > "Sort Methods Used": [ + > "top-N heapsort", + > diff --git a/src/test/regress/sql/incremental_sort.sql > b/src/test/regress/sql/incremental_sort.sql > index 2241cc9c02..6f70b36a81 100644 > --- a/src/test/regress/sql/incremental_sort.sql > +++ b/src/test/regress/sql/incremental_sort.sql > @@ -82,7 +82,7 @@ declare > space_key text; > begin > for node in select * from > jsonb_array_elements(explain_analyze_inc_sort_nodes(query)) t loop > - for group_key in select unnest(array['Full-sort Groups', 'Presorted > Groups']::text[]) t loop > + for group_key in select unnest(array['Full-sort Groups', 'Pre-sorted > Groups']::text[]) t loop > for space_key in select unnest(array['Sort Space Memory', 'Sort Space > Disk']::text[]) t loop > node := jsonb_set(node, array[group_key, space_key, 'Average Sort > Space Used'], '"NN"', false); > node := jsonb_set(node, array[group_key, space_key, 'Peak Sort Space > Used'], '"NN"', false); > @@ -105,7 +105,7 @@ declare > space_key text; > begin > for node in select * from > jsonb_array_elements(explain_analyze_inc_sort_nodes(query)) t loop > - for group_key in select unnest(array['Full-sort Groups', 'Presorted > Groups']::text[]) t loop > + for group_key in select unnest(array['Full-sort Groups', 'Pre-sorted > Groups']::text[]) t loop > group_stats := node->group_key; > for space_key in select unnest(array['Sort Space Memory', 'Sort Space > Disk']::text[]) t loop > if (group_stats->space_key->'Peak Sort Space Used')::bigint < > (group_stats->space_key->'Peak Sort Space Used')::bigint then > -- > 2.17.1 > -- Justin Pryzby System Administrator Telsasoft +1-952-707-8581
