suggest to rename enable_incrementalsort

Started by Peter Eisentrautover 5 years ago14 messages

peter.eisentraut@2ndquadrant.com

over 5 years ago

1 attachment(s)

I suggest to rename enable_incrementalsort to enable_incremental_sort.
This is obviously more readable and also how we have named recently
added multiword planner parameters.

See attached patch.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

0001-Rename-to-enable_incremental_sort-from-enable_increm.patchtext/plain; charset=UTF-8; name=0001-Rename-to-enable_incremental_sort-from-enable_increm.patch; x-mac-creator=0; x-mac-type=0Download

From 4c55bf802f42df5366d9c734bb9527d57de57de4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sun, 21 Jun 2020 08:21:53 +0200
Subject: [PATCH] Rename to enable_incremental_sort from enable_incrementalsort

---
 doc/src/sgml/config.sgml                          |  6 +++---
 doc/src/sgml/release-13.sgml                      |  4 ++--
 src/backend/optimizer/path/allpaths.c             |  2 +-
 src/backend/optimizer/path/costsize.c             |  2 +-
 src/backend/optimizer/plan/planner.c              | 14 +++++++-------
 src/backend/utils/misc/guc.c                      |  4 ++--
 src/backend/utils/misc/postgresql.conf.sample     |  2 +-
 src/include/optimizer/cost.h                      |  2 +-
 src/test/regress/expected/incremental_sort.out    |  4 ++--
 src/test/regress/expected/partition_aggregate.out |  2 +-
 src/test/regress/expected/sysviews.out            |  2 +-
 src/test/regress/sql/incremental_sort.sql         |  4 ++--
 src/test/regress/sql/partition_aggregate.sql      |  2 +-
 13 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index c6294df936..4c6daac86e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4591,10 +4591,10 @@ <title>Planner Method Configuration</title>
       </listitem>
      </varlistentry>
 
-     <varlistentry id="guc-enable-incrementalsort" xreflabel="enable_incrementalsort">
-      <term><varname>enable_incrementalsort</varname> (<type>boolean</type>)
+     <varlistentry id="guc-enable-incremental-sort" xreflabel="enable_incremental_sort">
+      <term><varname>enable_incremental_sort</varname> (<type>boolean</type>)
       <indexterm>
-       <primary><varname>enable_incrementalsort</varname> configuration parameter</primary>
+       <primary><varname>enable_incremental_sort</varname> configuration parameter</primary>
       </indexterm>
       </term>
       <listitem>
diff --git a/doc/src/sgml/release-13.sgml b/doc/src/sgml/release-13.sgml
index 401c557373..085766b8c4 100644
--- a/doc/src/sgml/release-13.sgml
+++ b/doc/src/sgml/release-13.sgml
@@ -588,7 +588,7 @@ <title>General Performance</title>
 -->
 
        <para>
-        Implement <link linkend="guc-enable-incrementalsort">incremental
+        Implement <link linkend="guc-enable-incremental-sort">incremental
         sorting</link> (James Coleman, Alexander Korotkov, Tomas Vondra)
        </para>
 
@@ -596,7 +596,7 @@ <title>General Performance</title>
         If a result is already sorted by several leading keys, this
         allows for batch sorting of additional trailing keys because the
         previous keys are already equal.  This is controlled by <xref
-        linkend="guc-enable-incrementalsort"/>.
+        linkend="guc-enable-incremental-sort"/>.
        </para>
       </listitem>
 
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index d984da25d7..81896de936 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2912,7 +2912,7 @@ generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_r
 			 * Consider incremental sort, but only when the subpath is already
 			 * partially sorted on a pathkey prefix.
 			 */
-			if (enable_incrementalsort && presorted_keys > 0)
+			if (enable_incremental_sort && presorted_keys > 0)
 			{
 				Path	   *tmp;
 
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 4ff3c7a2fd..87c9b49ce1 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -128,7 +128,7 @@ bool		enable_indexonlyscan = true;
 bool		enable_bitmapscan = true;
 bool		enable_tidscan = true;
 bool		enable_sort = true;
-bool		enable_incrementalsort = true;
+bool		enable_incremental_sort = true;
 bool		enable_hashagg = true;
 bool		hashagg_avoid_disk_plan = true;
 bool		enable_nestloop = true;
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 4131019fc9..14f3fd44e3 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5014,7 +5014,7 @@ create_ordered_paths(PlannerInfo *root,
 			 * presorted the path is. Additionally incremental sort may enable
 			 * a cheaper startup path to win out despite higher total cost.
 			 */
-			if (!enable_incrementalsort)
+			if (!enable_incremental_sort)
 				continue;
 
 			/* Likewise, if the path can't be used for incremental sort. */
@@ -5095,7 +5095,7 @@ create_ordered_paths(PlannerInfo *root,
 		 * sort_pathkeys because then we can't possibly have a presorted
 		 * prefix of the list without having the list be fully sorted.
 		 */
-		if (enable_incrementalsort && list_length(root->sort_pathkeys) > 1)
+		if (enable_incremental_sort && list_length(root->sort_pathkeys) > 1)
 		{
 			ListCell   *lc;
 
@@ -6572,7 +6572,7 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 			 * when the path is not already sorted and when incremental sort
 			 * is enabled.
 			 */
-			if (is_sorted || !enable_incrementalsort)
+			if (is_sorted || !enable_incremental_sort)
 				continue;
 
 			/* Restore the input path (we might have added Sort on top). */
@@ -6699,7 +6699,7 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 				 * when the path is not already sorted and when incremental
 				 * sort is enabled.
 				 */
-				if (is_sorted || !enable_incrementalsort)
+				if (is_sorted || !enable_incremental_sort)
 					continue;
 
 				/* Restore the input path (we might have added Sort on top). */
@@ -7022,7 +7022,7 @@ create_partial_grouping_paths(PlannerInfo *root,
 		 * group_pathkeys because then we can't possibly have a presorted
 		 * prefix of the list without having the list be fully sorted.
 		 */
-		if (enable_incrementalsort && list_length(root->group_pathkeys) > 1)
+		if (enable_incremental_sort && list_length(root->group_pathkeys) > 1)
 		{
 			foreach(lc, input_rel->pathlist)
 			{
@@ -7125,7 +7125,7 @@ create_partial_grouping_paths(PlannerInfo *root,
 			 * when the path is not already sorted and when incremental sort
 			 * is enabled.
 			 */
-			if (is_sorted || !enable_incrementalsort)
+			if (is_sorted || !enable_incremental_sort)
 				continue;
 
 			/* Restore the input path (we might have added Sort on top). */
@@ -7304,7 +7304,7 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
 	 * group_pathkeys because then we can't possibly have a presorted prefix
 	 * of the list without having the list be fully sorted.
 	 */
-	if (!enable_incrementalsort || list_length(root->group_pathkeys) == 1)
+	if (!enable_incremental_sort || list_length(root->group_pathkeys) == 1)
 		return;
 
 	/* also consider incremental sort on partial paths, if enabled */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fe7e2f8b91..9f5e223920 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -992,11 +992,11 @@ static struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 	{
-		{"enable_incrementalsort", PGC_USERSET, QUERY_TUNING_METHOD,
+		{"enable_incremental_sort", PGC_USERSET, QUERY_TUNING_METHOD,
 			gettext_noop("Enables the planner's use of incremental sort steps."),
 			NULL
 		},
-		&enable_incrementalsort,
+		&enable_incremental_sort,
 		true,
 		NULL, NULL, NULL
 	},
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index ac02bd0c00..bd4cb8144b 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -361,7 +361,7 @@
 #enable_parallel_append = on
 #enable_seqscan = on
 #enable_sort = on
-#enable_incrementalsort = on
+#enable_incremental_sort = on
 #enable_tidscan = on
 #enable_partitionwise_join = off
 #enable_partitionwise_aggregate = off
diff --git a/src/include/optimizer/cost.h b/src/include/optimizer/cost.h
index 92e70ec0d9..613db8eab6 100644
--- a/src/include/optimizer/cost.h
+++ b/src/include/optimizer/cost.h
@@ -53,7 +53,7 @@ extern PGDLLIMPORT bool enable_indexonlyscan;
 extern PGDLLIMPORT bool enable_bitmapscan;
 extern PGDLLIMPORT bool enable_tidscan;
 extern PGDLLIMPORT bool enable_sort;
-extern PGDLLIMPORT bool enable_incrementalsort;
+extern PGDLLIMPORT bool enable_incremental_sort;
 extern PGDLLIMPORT bool enable_hashagg;
 extern PGDLLIMPORT bool hashagg_avoid_disk_plan;
 extern PGDLLIMPORT bool enable_nestloop;
diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index 53accd0df9..e376ea1276 100644
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -1414,7 +1414,7 @@ create table t (a int, b int, c int);
 insert into t select mod(i,10),mod(i,10),i from generate_series(1,10000) s(i);
 create index on t (a);
 analyze t;
-set enable_incrementalsort = off;
+set enable_incremental_sort = off;
 explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1;
                       QUERY PLAN                      
 ------------------------------------------------------
@@ -1430,7 +1430,7 @@ explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1
                            ->  Parallel Seq Scan on t
 (10 rows)
 
-set enable_incrementalsort = on;
+set enable_incremental_sort = on;
 explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1;
                               QUERY PLAN                              
 ----------------------------------------------------------------------
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index c36970575f..45c698daf4 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -12,7 +12,7 @@ SET enable_partitionwise_join TO true;
 -- Disable parallel plans.
 SET max_parallel_workers_per_gather TO 0;
 -- Disable incremental sort, which can influence selected plans due to fuzz factor.
-SET enable_incrementalsort TO off;
+SET enable_incremental_sort TO off;
 --
 -- Tests for list partitioned tables.
 --
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 01b7786f01..06c4c3e476 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -76,7 +76,7 @@ select name, setting from pg_settings where name like 'enable%';
  enable_gathermerge             | on
  enable_hashagg                 | on
  enable_hashjoin                | on
- enable_incrementalsort         | on
+ enable_incremental_sort        | on
  enable_indexonlyscan           | on
  enable_indexscan               | on
  enable_material                | on
diff --git a/src/test/regress/sql/incremental_sort.sql b/src/test/regress/sql/incremental_sort.sql
index 373e62ac13..9c040c90e6 100644
--- a/src/test/regress/sql/incremental_sort.sql
+++ b/src/test/regress/sql/incremental_sort.sql
@@ -210,10 +210,10 @@
 create index on t (a);
 analyze t;
 
-set enable_incrementalsort = off;
+set enable_incremental_sort = off;
 explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1;
 
-set enable_incrementalsort = on;
+set enable_incremental_sort = on;
 explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1;
 
 -- Incremental sort vs. set operations with varno 0
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index 178f2079fa..117f65ecb4 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -13,7 +13,7 @@
 -- Disable parallel plans.
 SET max_parallel_workers_per_gather TO 0;
 -- Disable incremental sort, which can influence selected plans due to fuzz factor.
-SET enable_incrementalsort TO off;
+SET enable_incremental_sort TO off;
 
 --
 -- Tests for list partitioned tables.
-- 
2.27.0

Julien Rouhaud

rjuju123@gmail.com

over 5 years ago

In reply to: Peter Eisentraut (#1)

Re: suggest to rename enable_incrementalsort

On Sun, Jun 21, 2020 at 8:26 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

I suggest to rename enable_incrementalsort to enable_incremental_sort.
This is obviously more readable and also how we have named recently
added multiword planner parameters.

See attached patch.

+1, this is a way better name (and patch LGTM on REL_13_STABLE).

Tomas Vondra

tomas.vondra@2ndquadrant.com

over 5 years ago

In reply to: Julien Rouhaud (#2)

Re: suggest to rename enable_incrementalsort

On Sun, Jun 21, 2020 at 09:05:32AM +0200, Julien Rouhaud wrote:

On Sun, Jun 21, 2020 at 8:26 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

I suggest to rename enable_incrementalsort to enable_incremental_sort.
This is obviously more readable and also how we have named recently
added multiword planner parameters.

See attached patch.

+1, this is a way better name (and patch LGTM on REL_13_STABLE).

The reason why I kept the single-word variant is consistency with other
GUCs that affect planning, like enable_indexscan, enable_hashjoin and
many others.

That being said, I'm not particularly attached this choice, so if you
think this is better I'm OK with it.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

David Rowley

dgrowleyml@gmail.com

over 5 years ago

In reply to: Tomas Vondra (#3)

Re: suggest to rename enable_incrementalsort

On Sun, 21 Jun 2020 at 23:22, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:

On Sun, Jun 21, 2020 at 09:05:32AM +0200, Julien Rouhaud wrote:

On Sun, Jun 21, 2020 at 8:26 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

I suggest to rename enable_incrementalsort to enable_incremental_sort.
This is obviously more readable and also how we have named recently
added multiword planner parameters.

See attached patch.

+1, this is a way better name (and patch LGTM on REL_13_STABLE).

The reason why I kept the single-word variant is consistency with other
GUCs that affect planning, like enable_indexscan, enable_hashjoin and
many others.

Looking at the other enable_* GUCs, for all the ones that aim to
disable a certain executor node type, with the exception of
enable_hashagg and enable_bitmapscan, they're all pretty consistent in
naming the GUC after the executor node's .c file:

enable_bitmapscan nodeBitmapHeapscan.c
enable_gathermerge nodeGatherMerge.c
enable_hashagg nodeAgg.c
enable_hashjoin nodeHashjoin.c
enable_incrementalsort nodeIncrementalSort.c
enable_indexonlyscan nodeIndexonlyscan.c
enable_indexscan nodeIndexscan.c
enable_material nodeMaterial.c
enable_mergejoin nodeMergejoin.c
enable_nestloop nodeNestloop.c
enable_parallel_append nodeAppend.c
enable_parallel_hash nodeHash.c
enable_partition_pruning
enable_partitionwise_aggregate
enable_partitionwise_join
enable_seqscan nodeSeqscan.c
enable_sort nodeSort.c
enable_tidscan nodeTidscan.c

enable_partition_pruning, enable_partitionwise_aggregate,
enable_partitionwise_join are the odd ones out here as they're not
really related to a specific node type.

Going by that, it does seem the current name for
enable_incrementalsort is consistent with the majority. Naming it
enable_incremental_sort looks like it would be more suited if the
feature had been added by overloading nodeSort.c. In that regard, it
would be similar to enable_parallel_append and enable_parallel_hash,
where the middle word becomes a modifier.

David

Ashutosh Bapat

ashutosh.bapat.oss@gmail.com

over 5 years ago

In reply to: David Rowley (#4)

Re: suggest to rename enable_incrementalsort

On Mon, Jun 22, 2020 at 4:48 AM David Rowley <dgrowleyml@gmail.com> wrote:

On Sun, 21 Jun 2020 at 23:22, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:

On Sun, Jun 21, 2020 at 09:05:32AM +0200, Julien Rouhaud wrote:

On Sun, Jun 21, 2020 at 8:26 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

I suggest to rename enable_incrementalsort to enable_incremental_sort.
This is obviously more readable and also how we have named recently
added multiword planner parameters.

See attached patch.

+1, this is a way better name (and patch LGTM on REL_13_STABLE).

The reason why I kept the single-word variant is consistency with other
GUCs that affect planning, like enable_indexscan, enable_hashjoin and
many others.

Looking at the other enable_* GUCs, for all the ones that aim to
disable a certain executor node type, with the exception of
enable_hashagg and enable_bitmapscan, they're all pretty consistent in
naming the GUC after the executor node's .c file:

enable_bitmapscan nodeBitmapHeapscan.c
enable_gathermerge nodeGatherMerge.c
enable_hashagg nodeAgg.c
enable_hashjoin nodeHashjoin.c
enable_incrementalsort nodeIncrementalSort.c
enable_indexonlyscan nodeIndexonlyscan.c
enable_indexscan nodeIndexscan.c
enable_material nodeMaterial.c
enable_mergejoin nodeMergejoin.c
enable_nestloop nodeNestloop.c
enable_parallel_append nodeAppend.c
enable_parallel_hash nodeHash.c
enable_partition_pruning
enable_partitionwise_aggregate
enable_partitionwise_join
enable_seqscan nodeSeqscan.c
enable_sort nodeSort.c
enable_tidscan nodeTidscan.c

enable_partition_pruning, enable_partitionwise_aggregate,
enable_partitionwise_join are the odd ones out here as they're not
really related to a specific node type.

Thanks for the list. To me it's more of a question about readability
than consistency. enable_mergejoin, enable_hashjoin for example are
readable even without separating words merge_join or hash_join (many
times I have typed enable_hash_join and cursed :); but that was before
autocomplete was available). But enable_partitionwiseaggregate does
not look much different from enable_abracadabra :). Looking from that
angle, enable_incremental_sort is better than enable_incrementalsort.
We could have named enable_indexonlyscan as enable_index_only_scan for
better readability.

--
Best Wishes,
Ashutosh Bapat

Robert Haas

robertmhaas@gmail.com

over 5 years ago

In reply to: Tomas Vondra (#3)

Re: suggest to rename enable_incrementalsort

On Sun, Jun 21, 2020 at 7:22 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:

The reason why I kept the single-word variant is consistency with other
GUCs that affect planning, like enable_indexscan, enable_hashjoin and
many others.

Right, so that makes sense, but from a larger point of view, how much
sense does it actually make? I mean, I get the argument from tradition
and from internal naming consistency, but from a user perspective, why
does it makes sense for there to be underscores between some of the
words and not others? I think it just feels random, like someone is
charging us $1 per underscore so we're economizing.

So I'm +1 for changing this, and I'd definitely be +1 for renaming the
others if they weren't released already, and at least +0.5 for it
anyhow. It's bad enough that our source code has names_like_this and
NamesLikeThis and namesLikeThis; when we also start adding
names_likethis and NamesLike_this and maybe NaMeS___LiKeTh_is, I kind
of lose my mind. And avoiding that sort of thing in user-facing stuff
seems even more important.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tomas Vondra

tomas.vondra@2ndquadrant.com

over 5 years ago

In reply to: Robert Haas (#6)

Re: suggest to rename enable_incrementalsort

On Mon, Jun 22, 2020 at 10:16:54AM -0400, Robert Haas wrote:

On Sun, Jun 21, 2020 at 7:22 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:

The reason why I kept the single-word variant is consistency with other
GUCs that affect planning, like enable_indexscan, enable_hashjoin and
many others.

Right, so that makes sense, but from a larger point of view, how much
sense does it actually make? I mean, I get the argument from tradition
and from internal naming consistency, but from a user perspective, why
does it makes sense for there to be underscores between some of the
words and not others? I think it just feels random, like someone is
charging us $1 per underscore so we're economizing.

Sure. I'm not particularly attached to the current GUC, I've only tried
to explain that the naming was not entirely random. I agree having an
extra _ in the name would make it more readable.

So I'm +1 for changing this, and I'd definitely be +1 for renaming the
others if they weren't released already, and at least +0.5 for it
anyhow. It's bad enough that our source code has names_like_this and
NamesLikeThis and namesLikeThis; when we also start adding
names_likethis and NamesLike_this and maybe NaMeS___LiKeTh_is, I kind
of lose my mind. And avoiding that sort of thing in user-facing stuff
seems even more important.

OK, challenge accepted. $100 to the first person who commits a patch
with a variable NaMeS___LiKeTh_is.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Tom Lane

tgl@sss.pgh.pa.us

over 5 years ago

In reply to: Robert Haas (#6)

Re: suggest to rename enable_incrementalsort

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Jun 21, 2020 at 7:22 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:

The reason why I kept the single-word variant is consistency with other
GUCs that affect planning, like enable_indexscan, enable_hashjoin and
many others.

Right, so that makes sense, but from a larger point of view, how much
sense does it actually make?

Maybe I'm just used to the names, but I find that things like
"enable_seqscan" and "enable_nestloop" are pretty readable.
Once they get longer, though, not so much. So I agree with
renaming enable_incrementalsort.

So I'm +1 for changing this, and I'd definitely be +1 for renaming the
others if they weren't released already, and at least +0.5 for it
anyhow.

Nah. Those names are way too well entrenched. Besides which, if
we open them up for reconsideration, there's going to be a lot of
bikeshedding done. Should "enable_seqscan" become "enable_seq_scan",
or "enable_sequential_scan", or maybe "enable_scan_sequential"?
Why doesn't "enable_nestloop" contain the word "join"? Etc etc.

(I do have to wonder if maybe this one should be enable_sort_incremental.)

regards, tom lane

Robert Haas

robertmhaas@gmail.com

over 5 years ago

In reply to: Tomas Vondra (#7)

Re: suggest to rename enable_incrementalsort

On Mon, Jun 22, 2020 at 10:31 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:

OK, challenge accepted. $100 to the first person who commits a patch
with a variable NaMeS___LiKeTh_is.

:-)

Well, that was hyperbole, but people have proposed some pretty wacky
schemes, and a few of those have ended up in the tree. For example we
have AtEOXact_PgStat and its close friend AtEOXact_on_commit_actions,
for instance, or out_gistxlogDelete, or
IncrementVarSublevelsUp_rtable, or convert_EXISTS_sublink_to_join. I
confess haven't managed to find any plausible examples of underscores
in the middle of a word yet, and we only have a handful of examples of
double-underscore and none with triple-underscore, but we've got
nearly every combination of lower-case words, upper-case words,
initial-capital words, underscores separating words or not, and words
abbreviated or not, and it's not hard to find cases where several
different styles are used in the same identifier. This isn't the end
of the world or anything, but I think we would be better off if we
tried to do less of it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#10

Bruce Momjian

bruce@momjian.us

over 5 years ago

In reply to: Tom Lane (#8)

Re: suggest to rename enable_incrementalsort

On Mon, Jun 22, 2020 at 10:41:17AM -0400, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Jun 21, 2020 at 7:22 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:

The reason why I kept the single-word variant is consistency with other
GUCs that affect planning, like enable_indexscan, enable_hashjoin and
many others.

Right, so that makes sense, but from a larger point of view, how much
sense does it actually make?

Maybe I'm just used to the names, but I find that things like
"enable_seqscan" and "enable_nestloop" are pretty readable.
Once they get longer, though, not so much. So I agree with
renaming enable_incrementalsort.

I think the big problem is that, without the extra underscore, it reads
as increment-alsort. ;-)

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

#11

Tom Lane

tgl@sss.pgh.pa.us

over 5 years ago

In reply to: Bruce Momjian (#10)

Re: suggest to rename enable_incrementalsort

Bruce Momjian <bruce@momjian.us> writes:

On Mon, Jun 22, 2020 at 10:41:17AM -0400, Tom Lane wrote:

Maybe I'm just used to the names, but I find that things like
"enable_seqscan" and "enable_nestloop" are pretty readable.
Once they get longer, though, not so much. So I agree with
renaming enable_incrementalsort.

I think the big problem is that, without the extra underscore, it reads
as increment-alsort. ;-)

Yeah, the longer the name gets, the harder it is to see where the
word boundaries are.

regards, tom lane

#12

Robert Haas

robertmhaas@gmail.com

over 5 years ago

In reply to: Bruce Momjian (#10)

Re: suggest to rename enable_incrementalsort

On Mon, Jun 22, 2020 at 11:22 AM Bruce Momjian <bruce@momjian.us> wrote:

I think the big problem is that, without the extra underscore, it reads
as increment-alsort. ;-)

I know you're joking, but I think there's a serious issue here. We
often both omit word separators and also abbreviate, and I doubt that
the meaning is always obvious to people whose first language is
Japanese or Russian or something. The only human language other than
English in which I have any competence at all is Spanish, and if
somebody speaks Spanish to me the way that it's explained in a
textbook, I can understand it fairly well, especially if we're talking
about the kinds of topics that textbooks discuss rather than technical
stuff. But as soon as you start to use abbreviations or idioms, you're
going to lose me. Without a doubt, the best solution to this problem
would be for me to have better Spanish, but in the absence of that, on
those occasions when I need to communicate in Spanish, I sure do like
it when people are willing and able to make that as easy for me as
they can. I suspect other people have similar experiences.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#13

James Coleman

jtc331@gmail.com

over 5 years ago

In reply to: Robert Haas (#12)

1 attachment(s)

Re: suggest to rename enable_incrementalsort

I think the change makes a lot of sense. The only reason I had it as
enable_incrementalsort in the first place was trying to broadly
following the existing GUC names, but as has already been pointed out,
there's a lot of variation there, and my version of the patch already
changed it to be more readable (at one point it was
enable_incsort...which is short...but does not have an obvious
meaning).

I've attached a patch to make the change, though if people are
interested in Tom's suggestion of enable_sort_incremental I could
switch to that.

James

Attachments:

v1-0001-Rename-enable_incrementalsort-for-clarity.patchapplication/octet-stream; name=v1-0001-Rename-enable_incrementalsort-for-clarity.patchDownload

From 47f5e6221b2fa8e857d154ed0f0529c9254c5ca1 Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Thu, 2 Jul 2020 15:20:58 +0000
Subject: [PATCH v1] Rename enable_incrementalsort for clarity

---
 doc/src/sgml/config.sgml                          |  6 +++---
 src/backend/optimizer/path/allpaths.c             |  2 +-
 src/backend/optimizer/path/costsize.c             |  2 +-
 src/backend/optimizer/plan/planner.c              | 14 +++++++-------
 src/backend/utils/misc/guc.c                      |  4 ++--
 src/backend/utils/misc/postgresql.conf.sample     |  2 +-
 src/include/optimizer/cost.h                      |  2 +-
 src/test/regress/expected/incremental_sort.out    |  4 ++--
 src/test/regress/expected/partition_aggregate.out |  2 +-
 src/test/regress/expected/sysviews.out            |  2 +-
 src/test/regress/sql/incremental_sort.sql         |  4 ++--
 src/test/regress/sql/partition_aggregate.sql      |  2 +-
 12 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b81aab239f..c4d5e8f3e4 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4574,10 +4574,10 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
       </listitem>
      </varlistentry>
 
-     <varlistentry id="guc-enable-incrementalsort" xreflabel="enable_incrementalsort">
-      <term><varname>enable_incrementalsort</varname> (<type>boolean</type>)
+     <varlistentry id="guc-enable-incrementalsort" xreflabel="enable_incremental_sort">
+      <term><varname>enable_incremental_sort</varname> (<type>boolean</type>)
       <indexterm>
-       <primary><varname>enable_incrementalsort</varname> configuration parameter</primary>
+       <primary><varname>enable_incremental_sort</varname> configuration parameter</primary>
       </indexterm>
       </term>
       <listitem>
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index d984da25d7..81896de936 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2912,7 +2912,7 @@ generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_r
 			 * Consider incremental sort, but only when the subpath is already
 			 * partially sorted on a pathkey prefix.
 			 */
-			if (enable_incrementalsort && presorted_keys > 0)
+			if (enable_incremental_sort && presorted_keys > 0)
 			{
 				Path	   *tmp;
 
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 4ff3c7a2fd..87c9b49ce1 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -128,7 +128,7 @@ bool		enable_indexonlyscan = true;
 bool		enable_bitmapscan = true;
 bool		enable_tidscan = true;
 bool		enable_sort = true;
-bool		enable_incrementalsort = true;
+bool		enable_incremental_sort = true;
 bool		enable_hashagg = true;
 bool		hashagg_avoid_disk_plan = true;
 bool		enable_nestloop = true;
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 4131019fc9..14f3fd44e3 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5014,7 +5014,7 @@ create_ordered_paths(PlannerInfo *root,
 			 * presorted the path is. Additionally incremental sort may enable
 			 * a cheaper startup path to win out despite higher total cost.
 			 */
-			if (!enable_incrementalsort)
+			if (!enable_incremental_sort)
 				continue;
 
 			/* Likewise, if the path can't be used for incremental sort. */
@@ -5095,7 +5095,7 @@ create_ordered_paths(PlannerInfo *root,
 		 * sort_pathkeys because then we can't possibly have a presorted
 		 * prefix of the list without having the list be fully sorted.
 		 */
-		if (enable_incrementalsort && list_length(root->sort_pathkeys) > 1)
+		if (enable_incremental_sort && list_length(root->sort_pathkeys) > 1)
 		{
 			ListCell   *lc;
 
@@ -6572,7 +6572,7 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 			 * when the path is not already sorted and when incremental sort
 			 * is enabled.
 			 */
-			if (is_sorted || !enable_incrementalsort)
+			if (is_sorted || !enable_incremental_sort)
 				continue;
 
 			/* Restore the input path (we might have added Sort on top). */
@@ -6699,7 +6699,7 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 				 * when the path is not already sorted and when incremental
 				 * sort is enabled.
 				 */
-				if (is_sorted || !enable_incrementalsort)
+				if (is_sorted || !enable_incremental_sort)
 					continue;
 
 				/* Restore the input path (we might have added Sort on top). */
@@ -7022,7 +7022,7 @@ create_partial_grouping_paths(PlannerInfo *root,
 		 * group_pathkeys because then we can't possibly have a presorted
 		 * prefix of the list without having the list be fully sorted.
 		 */
-		if (enable_incrementalsort && list_length(root->group_pathkeys) > 1)
+		if (enable_incremental_sort && list_length(root->group_pathkeys) > 1)
 		{
 			foreach(lc, input_rel->pathlist)
 			{
@@ -7125,7 +7125,7 @@ create_partial_grouping_paths(PlannerInfo *root,
 			 * when the path is not already sorted and when incremental sort
 			 * is enabled.
 			 */
-			if (is_sorted || !enable_incrementalsort)
+			if (is_sorted || !enable_incremental_sort)
 				continue;
 
 			/* Restore the input path (we might have added Sort on top). */
@@ -7304,7 +7304,7 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
 	 * group_pathkeys because then we can't possibly have a presorted prefix
 	 * of the list without having the list be fully sorted.
 	 */
-	if (!enable_incrementalsort || list_length(root->group_pathkeys) == 1)
+	if (!enable_incremental_sort || list_length(root->group_pathkeys) == 1)
 		return;
 
 	/* also consider incremental sort on partial paths, if enabled */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 75fc6f11d6..3a802d8627 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -983,11 +983,11 @@ static struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 	{
-		{"enable_incrementalsort", PGC_USERSET, QUERY_TUNING_METHOD,
+		{"enable_incremental_sort", PGC_USERSET, QUERY_TUNING_METHOD,
 			gettext_noop("Enables the planner's use of incremental sort steps."),
 			NULL
 		},
-		&enable_incrementalsort,
+		&enable_incremental_sort,
 		true,
 		NULL, NULL, NULL
 	},
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 3a25287a39..0d98e546a6 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -361,7 +361,7 @@
 #enable_parallel_append = on
 #enable_seqscan = on
 #enable_sort = on
-#enable_incrementalsort = on
+#enable_incremental_sort = on
 #enable_tidscan = on
 #enable_partitionwise_join = off
 #enable_partitionwise_aggregate = off
diff --git a/src/include/optimizer/cost.h b/src/include/optimizer/cost.h
index 92e70ec0d9..613db8eab6 100644
--- a/src/include/optimizer/cost.h
+++ b/src/include/optimizer/cost.h
@@ -53,7 +53,7 @@ extern PGDLLIMPORT bool enable_indexonlyscan;
 extern PGDLLIMPORT bool enable_bitmapscan;
 extern PGDLLIMPORT bool enable_tidscan;
 extern PGDLLIMPORT bool enable_sort;
-extern PGDLLIMPORT bool enable_incrementalsort;
+extern PGDLLIMPORT bool enable_incremental_sort;
 extern PGDLLIMPORT bool enable_hashagg;
 extern PGDLLIMPORT bool hashagg_avoid_disk_plan;
 extern PGDLLIMPORT bool enable_nestloop;
diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index 53accd0df9..e376ea1276 100644
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -1414,7 +1414,7 @@ create table t (a int, b int, c int);
 insert into t select mod(i,10),mod(i,10),i from generate_series(1,10000) s(i);
 create index on t (a);
 analyze t;
-set enable_incrementalsort = off;
+set enable_incremental_sort = off;
 explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1;
                       QUERY PLAN                      
 ------------------------------------------------------
@@ -1430,7 +1430,7 @@ explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1
                            ->  Parallel Seq Scan on t
 (10 rows)
 
-set enable_incrementalsort = on;
+set enable_incremental_sort = on;
 explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1;
                               QUERY PLAN                              
 ----------------------------------------------------------------------
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index c36970575f..45c698daf4 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -12,7 +12,7 @@ SET enable_partitionwise_join TO true;
 -- Disable parallel plans.
 SET max_parallel_workers_per_gather TO 0;
 -- Disable incremental sort, which can influence selected plans due to fuzz factor.
-SET enable_incrementalsort TO off;
+SET enable_incremental_sort TO off;
 --
 -- Tests for list partitioned tables.
 --
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 01b7786f01..06c4c3e476 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -76,7 +76,7 @@ select name, setting from pg_settings where name like 'enable%';
  enable_gathermerge             | on
  enable_hashagg                 | on
  enable_hashjoin                | on
- enable_incrementalsort         | on
+ enable_incremental_sort        | on
  enable_indexonlyscan           | on
  enable_indexscan               | on
  enable_material                | on
diff --git a/src/test/regress/sql/incremental_sort.sql b/src/test/regress/sql/incremental_sort.sql
index 373e62ac13..9c040c90e6 100644
--- a/src/test/regress/sql/incremental_sort.sql
+++ b/src/test/regress/sql/incremental_sort.sql
@@ -210,10 +210,10 @@ insert into t select mod(i,10),mod(i,10),i from generate_series(1,10000) s(i);
 create index on t (a);
 analyze t;
 
-set enable_incrementalsort = off;
+set enable_incremental_sort = off;
 explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1;
 
-set enable_incrementalsort = on;
+set enable_incremental_sort = on;
 explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1;
 
 -- Incremental sort vs. set operations with varno 0
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index 178f2079fa..117f65ecb4 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -13,7 +13,7 @@ SET enable_partitionwise_join TO true;
 -- Disable parallel plans.
 SET max_parallel_workers_per_gather TO 0;
 -- Disable incremental sort, which can influence selected plans due to fuzz factor.
-SET enable_incrementalsort TO off;
+SET enable_incremental_sort TO off;
 
 --
 -- Tests for list partitioned tables.
-- 
2.20.1

#14

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 5 years ago

In reply to: James Coleman (#13)

Re: suggest to rename enable_incrementalsort

On 2020-07-02 17:25, James Coleman wrote:

I think the change makes a lot of sense. The only reason I had it as
enable_incrementalsort in the first place was trying to broadly
following the existing GUC names, but as has already been pointed out,
there's a lot of variation there, and my version of the patch already
changed it to be more readable (at one point it was
enable_incsort...which is short...but does not have an obvious
meaning).

I've attached a patch to make the change,

committed

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services