Forking this thread
https://www.postgresql.org/message-id/20210403154336.GG29125%40momjian.us

On Sat, Apr  3, 2021 at 08:38:18PM +0530, aditya desai wrote:
> > > >> Yes, force_parallel_mode is on. Should we set it off?

Bruce Momjian <br...@momjian.us> writes:
> > > > Yes.  I bet someone set it without reading our docs:
...
> > > > We might need to clarify this sentence to be clearer it is _only_ for
> > > > testing.

On Sat, Apr 03, 2021 at 11:39:19AM -0400, Tom Lane wrote:
> > > I wonder why it is listed under planner options at all, and not under
> > > developer options.

On Sat, Apr  3, 2021 at 10:41:14AM -0500, Justin Pryzby wrote:
> > Because it's there to help DBAs catch errors in functions incorrectly 
> > marked as
> > parallel safe.

On Sat, Apr 03, 2021 at 11:43:36AM -0400, Bruce Momjian wrote:
> Uh, isn't that developer/debugging?

I understood "developer" to mean someone who's debugging postgres itself, not
(say) a function written using pl/pgsql.  Like backtrace_functions,
post_auth_delay, jit_profiling_support.

But I see that some "dev" options are more user-facing (for a sufficiently
advanced user):
ignore_checksum_failure, ignore_invalid_pages, zero_damaged_pages.

Also, I understood this to mean the "category" in pg_settings, but I guess
what's important here is the absense of the GUC in the sample/template config
file.  pg_settings.category and the sample headings it appears are intended to
be synchronized, but a few of them are out of sync.  See attached.

+1 to move this to "developer" options and remove it from the sample config:

# - Other Planner Options -
#force_parallel_mode = off

-- 
Justin
>From ce0c5b99650859bb13f204ea56221ca854a601e1 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sat, 3 Apr 2021 19:06:37 -0500
Subject: [PATCH 1/4] track_activity_query_size is STATS_COLLECTOR category

Not Resource Usage / Memory, as since 995fb7420
---
 contrib/postgres_fdw/postgres_fdw.c | 1 +
 src/backend/utils/misc/guc.c        | 2 +-
 src/include/pg_config_manual.h      | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 16c2979f2d..aff6e8c085 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -6797,6 +6797,7 @@ fetch_more_data_begin(AsyncRequest *areq)
 	char		sql[64];
 
 	Assert(!fsstate->conn_state->pendingAreq);
+	Assert(fsstate->conn);
 
 	/* Create the cursor synchronously. */
 	if (!fsstate->cursor_exists)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 584daffc8a..c315dd4bc1 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3435,7 +3435,7 @@ static struct config_int ConfigureNamesInt[] =
 	},
 
 	{
-		{"track_activity_query_size", PGC_POSTMASTER, RESOURCES_MEM,
+		{"track_activity_query_size", PGC_POSTMASTER, STATS_COLLECTOR,
 			gettext_noop("Sets the size reserved for pg_stat_activity.query, in bytes."),
 			NULL,
 			GUC_UNIT_BYTE
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index e28c990382..fdb4fc1de2 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -288,7 +288,7 @@
  * You should normally use MEMORY_CONTEXT_CHECKING with USE_VALGRIND;
  * instrumentation of repalloc() is inferior without it.
  */
-/* #define USE_VALGRIND */
+#define USE_VALGRIND
 
 /*
  * Define this to cause pfree()'d memory to be cleared immediately, to
@@ -313,7 +313,7 @@
  * facilitate catching code that depends on the contents of uninitialized
  * memory.  Caution: this is horrendously expensive.
  */
-/* #define RANDOMIZE_ALLOCATED_MEMORY */
+#define RANDOMIZE_ALLOCATED_MEMORY
 
 /*
  * For cache invalidation debugging, define CLOBBER_CACHE_ENABLED to enable
-- 
2.17.0

>From 6604d8b515ef62e77fea2550e5f040d89fa948f4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sat, 3 Apr 2021 19:10:01 -0500
Subject: [PATCH 2/4] log_autovacuum_min_duration is LOGGING_WHAT

Not AUTOVACUUM, since 48f7e6439 and ef23a7744
---
 doc/src/sgml/config.sgml                      | 56 +++++++++----------
 src/backend/utils/misc/postgresql.conf.sample |  8 +--
 2 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 9d87b5097a..c4d5126c2a 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6688,6 +6688,34 @@ local0.*    /var/log/postgresql
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-log-autovacuum-min-duration" xreflabel="log_autovacuum_min_duration">
+      <term><varname>log_autovacuum_min_duration</varname> (<type>integer</type>)
+      <indexterm>
+       <primary><varname>log_autovacuum_min_duration</varname></primary>
+       <secondary>configuration parameter</secondary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Causes each action executed by autovacuum to be logged if it ran for at
+        least the specified amount of time.  Setting this to zero logs
+        all autovacuum actions. <literal>-1</literal> (the default) disables
+        logging autovacuum actions.
+        If this value is specified without units, it is taken as milliseconds.
+        For example, if you set this to
+        <literal>250ms</literal> then all automatic vacuums and analyzes that run
+        250ms or longer will be logged.  In addition, when this parameter is
+        set to any value other than <literal>-1</literal>, a message will be
+        logged if an autovacuum action is skipped due to a conflicting lock or a
+        concurrently dropped relation.  Enabling this parameter can be helpful
+        in tracking autovacuum activity.  This parameter can only be set in
+        the <filename>postgresql.conf</filename> file or on the server command line;
+        but the setting can be overridden for individual tables by
+        changing table storage parameters.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-log-checkpoints" xreflabel="log_checkpoints">
       <term><varname>log_checkpoints</varname> (<type>boolean</type>)
       <indexterm>
@@ -7662,34 +7690,6 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
       </listitem>
      </varlistentry>
 
-     <varlistentry id="guc-log-autovacuum-min-duration" xreflabel="log_autovacuum_min_duration">
-      <term><varname>log_autovacuum_min_duration</varname> (<type>integer</type>)
-      <indexterm>
-       <primary><varname>log_autovacuum_min_duration</varname></primary>
-       <secondary>configuration parameter</secondary>
-      </indexterm>
-      </term>
-      <listitem>
-       <para>
-        Causes each action executed by autovacuum to be logged if it ran for at
-        least the specified amount of time.  Setting this to zero logs
-        all autovacuum actions. <literal>-1</literal> (the default) disables
-        logging autovacuum actions.
-        If this value is specified without units, it is taken as milliseconds.
-        For example, if you set this to
-        <literal>250ms</literal> then all automatic vacuums and analyzes that run
-        250ms or longer will be logged.  In addition, when this parameter is
-        set to any value other than <literal>-1</literal>, a message will be
-        logged if an autovacuum action is skipped due to a conflicting lock or a
-        concurrently dropped relation.  Enabling this parameter can be helpful
-        in tracking autovacuum activity.  This parameter can only be set in
-        the <filename>postgresql.conf</filename> file or on the server command line;
-        but the setting can be overridden for individual tables by
-        changing table storage parameters.
-       </para>
-      </listitem>
-     </varlistentry>
-
      <varlistentry id="guc-autovacuum-max-workers" xreflabel="autovacuum_max_workers">
       <term><varname>autovacuum_max_workers</varname> (<type>integer</type>)
       <indexterm>
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 30cfddac1f..52c4373a79 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -568,6 +568,10 @@
 #log_temp_files = -1			# log temporary files equal or larger
 					# than the specified size in kilobytes;
 					# -1 disables, 0 logs all temp files
+#log_autovacuum_min_duration = -1	# -1 disables, 0 logs all actions and
+					# their durations, > 0 logs only
+					# actions running at least this number
+					# of milliseconds.
 #log_timezone = 'GMT'
 
 #------------------------------------------------------------------------------
@@ -608,10 +612,6 @@
 
 #autovacuum = on			# Enable autovacuum subprocess?  'on'
 					# requires track_counts to also be on.
-#log_autovacuum_min_duration = -1	# -1 disables, 0 logs all actions and
-					# their durations, > 0 logs only
-					# actions running at least this number
-					# of milliseconds.
 #autovacuum_max_workers = 3		# max number of autovacuum subprocesses
 					# (change requires restart)
 #autovacuum_naptime = 1min		# time between autovacuum runs
-- 
2.17.0

>From b386aceaece02b57b2c5c5640bbf38d9da99a5e4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sat, 3 Apr 2021 19:17:03 -0500
Subject: [PATCH 3/4] track_commit_timestamp is REPLICATION_SENDING

If I'm not wrong, this was missed at 4bd8ed31b
---
 src/backend/utils/misc/guc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index c315dd4bc1..27287599ad 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1180,7 +1180,7 @@ static struct config_bool ConfigureNamesBool[] =
 		check_bonjour, NULL, NULL
 	},
 	{
-		{"track_commit_timestamp", PGC_POSTMASTER, REPLICATION,
+		{"track_commit_timestamp", PGC_POSTMASTER, REPLICATION_SENDING,
 			gettext_noop("Collects transaction commit time."),
 			NULL
 		},
-- 
2.17.0

>From cd224c1524d60c9b5679a853358b59dad00a35ca Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sat, 3 Apr 2021 19:24:50 -0500
Subject: [PATCH 4/4] Change force_parallel_mode to a DEVELOPER GUC, and remove
 it from sample config..

..to help avoid users finding this option and changing it in hopes that it'll
make their queries faster, but without reading the documentation or
understanding what it does.
---
 doc/src/sgml/config.sgml                      | 90 +++++++++----------
 src/backend/utils/misc/guc.c                  |  2 +-
 src/backend/utils/misc/postgresql.conf.sample |  1 -
 3 files changed, 46 insertions(+), 47 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index c4d5126c2a..5c7c5a39ca 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5788,51 +5788,6 @@ SELECT * FROM parent WHERE key = 2400;
       </listitem>
      </varlistentry>
 
-     <varlistentry id="guc-force-parallel-mode" xreflabel="force_parallel_mode">
-      <term><varname>force_parallel_mode</varname> (<type>enum</type>)
-      <indexterm>
-       <primary><varname>force_parallel_mode</varname> configuration parameter</primary>
-      </indexterm>
-      </term>
-      <listitem>
-       <para>
-        Allows the use of parallel queries for testing purposes even in cases
-        where no performance benefit is expected.
-        The allowed values of <varname>force_parallel_mode</varname> are
-        <literal>off</literal> (use parallel mode only when it is expected to improve
-        performance), <literal>on</literal> (force parallel query for all queries
-        for which it is thought to be safe), and <literal>regress</literal> (like
-        <literal>on</literal>, but with additional behavior changes as explained
-        below).
-       </para>
-
-       <para>
-        More specifically, setting this value to <literal>on</literal> will add
-        a <literal>Gather</literal> node to the top of any query plan for which this
-        appears to be safe, so that the query runs inside of a parallel worker.
-        Even when a parallel worker is not available or cannot be used,
-        operations such as starting a subtransaction that would be prohibited
-        in a parallel query context will be prohibited unless the planner
-        believes that this will cause the query to fail.  If failures or
-        unexpected results occur when this option is set, some functions used
-        by the query may need to be marked <literal>PARALLEL UNSAFE</literal>
-        (or, possibly, <literal>PARALLEL RESTRICTED</literal>).
-       </para>
-
-       <para>
-        Setting this value to <literal>regress</literal> has all of the same effects
-        as setting it to <literal>on</literal> plus some additional effects that are
-        intended to facilitate automated regression testing.  Normally,
-        messages from a parallel worker include a context line indicating that,
-        but a setting of <literal>regress</literal> suppresses this line so that the
-        output is the same as in non-parallel execution.  Also,
-        the <literal>Gather</literal> nodes added to plans by this setting are hidden
-        in <literal>EXPLAIN</literal> output so that the output matches what
-        would be obtained if this setting were turned <literal>off</literal>.
-       </para>
-      </listitem>
-     </varlistentry>
-
      <varlistentry id="guc-plan-cache_mode" xreflabel="plan_cache_mode">
       <term><varname>plan_cache_mode</varname> (<type>enum</type>)
       <indexterm>
@@ -10231,6 +10186,51 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-force-parallel-mode" xreflabel="force_parallel_mode">
+      <term><varname>force_parallel_mode</varname> (<type>enum</type>)
+      <indexterm>
+       <primary><varname>force_parallel_mode</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Allows the use of parallel queries for testing purposes even in cases
+        where no performance benefit is expected.
+        The allowed values of <varname>force_parallel_mode</varname> are
+        <literal>off</literal> (use parallel mode only when it is expected to improve
+        performance), <literal>on</literal> (force parallel query for all queries
+        for which it is thought to be safe), and <literal>regress</literal> (like
+        <literal>on</literal>, but with additional behavior changes as explained
+        below).
+       </para>
+
+       <para>
+        More specifically, setting this value to <literal>on</literal> will add
+        a <literal>Gather</literal> node to the top of any query plan for which this
+        appears to be safe, so that the query runs inside of a parallel worker.
+        Even when a parallel worker is not available or cannot be used,
+        operations such as starting a subtransaction that would be prohibited
+        in a parallel query context will be prohibited unless the planner
+        believes that this will cause the query to fail.  If failures or
+        unexpected results occur when this option is set, some functions used
+        by the query may need to be marked <literal>PARALLEL UNSAFE</literal>
+        (or, possibly, <literal>PARALLEL RESTRICTED</literal>).
+       </para>
+
+       <para>
+        Setting this value to <literal>regress</literal> has all of the same effects
+        as setting it to <literal>on</literal> plus some additional effects that are
+        intended to facilitate automated regression testing.  Normally,
+        messages from a parallel worker include a context line indicating that,
+        but a setting of <literal>regress</literal> suppresses this line so that the
+        output is the same as in non-parallel execution.  Also,
+        the <literal>Gather</literal> nodes added to plans by this setting are hidden
+        in <literal>EXPLAIN</literal> output so that the output matches what
+        would be obtained if this setting were turned <literal>off</literal>.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-ignore-system-indexes" xreflabel="ignore_system_indexes">
       <term><varname>ignore_system_indexes</varname> (<type>boolean</type>)
       <indexterm>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 27287599ad..06de7c0308 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4840,7 +4840,7 @@ static struct config_enum ConfigureNamesEnum[] =
 	},
 
 	{
-		{"force_parallel_mode", PGC_USERSET, QUERY_TUNING_OTHER,
+		{"force_parallel_mode", PGC_USERSET, DEVELOPER_OPTIONS,
 			gettext_noop("Forces use of parallel query facilities."),
 			gettext_noop("If possible, run query using a parallel worker and with parallel restrictions."),
 			GUC_EXPLAIN
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 52c4373a79..50faa50462 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -415,7 +415,6 @@
 #from_collapse_limit = 8
 #join_collapse_limit = 8		# 1 disables collapsing of explicit
 					# JOIN clauses
-#force_parallel_mode = off
 #jit = on				# allow JIT compilation
 #plan_cache_mode = auto			# auto, force_generic_plan or
 					# force_custom_plan
-- 
2.17.0

Reply via email to