Re: is_superuser versus set_config_option's parallelism check - Mailing list pgsql-hackers

I wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> Another option might be to introduce a new GUC flag or source for anything
>> we want to bypass the check (perhaps with the stipulation that it must also
>> be marked PGC_INTERNAL).

> A new GUC flag seems like a promising approach, and better than
> giving a blanket exemption to PGC_INTERNAL.  At least for
> is_superuser, there's no visible value in restricting which
> SetConfigOption calls can change it; they'd all need the escape
> hatch.

Here's a draft patch to fix it with a flag, now with regression tests.

Also, now that the error depends on which parameter we're talking
about, I thought it best to include the parameter name in the error
and to re-word it to be more like all the other can't-set-this-now
errors just below it.  I'm half tempted to change the errcode and
set_config_option return value to match the others too, ie
ERRCODE_CANT_CHANGE_RUNTIME_PARAM and "return 0" not -1.
I don't think the existing choices here are very well thought
through, and they're certainly inconsistent with a lot of
otherwise-similar-seeming refusals in set_config_option.

            regards, tom lane

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0c593b81b4..7dd7b2ec4a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3428,6 +3428,16 @@ set_config_with_handle(const char *name, config_handle *handle,
             elevel = ERROR;
     }

+    /* if handle is specified, no need to look up option */
+    if (!handle)
+    {
+        record = find_option(name, true, false, elevel);
+        if (record == NULL)
+            return 0;
+    }
+    else
+        record = handle;
+
     /*
      * GUC_ACTION_SAVE changes are acceptable during a parallel operation,
      * because the current worker will also pop the change.  We're probably
@@ -3435,26 +3445,20 @@ set_config_with_handle(const char *name, config_handle *handle,
      * body should observe the change, and peer workers do not share in the
      * execution of a function call started by this worker.
      *
+     * Also allow normal setting if the GUC is marked GUC_ALLOW_IN_PARALLEL.
+     *
      * Other changes might need to affect other workers, so forbid them.
      */
-    if (IsInParallelMode() && changeVal && action != GUC_ACTION_SAVE)
+    if (IsInParallelMode() && changeVal && action != GUC_ACTION_SAVE &&
+        (record->flags & GUC_ALLOW_IN_PARALLEL) == 0)
     {
         ereport(elevel,
                 (errcode(ERRCODE_INVALID_TRANSACTION_STATE),
-                 errmsg("cannot set parameters during a parallel operation")));
+                 errmsg("parameter \"%s\" cannot be set during a parallel operation",
+                        name)));
         return -1;
     }

-    /* if handle is specified, no need to look up option */
-    if (!handle)
-    {
-        record = find_option(name, true, false, elevel);
-        if (record == NULL)
-            return 0;
-    }
-    else
-        record = handle;
-
     /*
      * Check if the option can be set at this time. See guc.h for the precise
      * rules.
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index c0a52cdcc3..1c4392dd22 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -1015,7 +1015,7 @@ struct config_bool ConfigureNamesBool[] =
         {"is_superuser", PGC_INTERNAL, UNGROUPED,
             gettext_noop("Shows whether the current user is a superuser."),
             NULL,
-            GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+            GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE |
GUC_ALLOW_IN_PARALLEL
         },
         ¤t_role_is_superuser,
         false,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 4129ea37ee..ba6883ae8f 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -223,6 +223,7 @@ typedef enum
 #define GUC_DISALLOW_IN_AUTO_FILE \
                                0x002000 /* can't set in PG_AUTOCONF_FILENAME */
 #define GUC_RUNTIME_COMPUTED   0x004000 /* delay processing in 'postgres -C' */
+#define GUC_ALLOW_IN_PARALLEL  0x008000 /* allow setting in parallel mode */

 #define GUC_UNIT_KB             0x01000000 /* value is in kilobytes */
 #define GUC_UNIT_BLOCKS         0x02000000 /* value is in blocks */
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 68a629619a..a61c138bd0 100644
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -1347,3 +1347,36 @@ select current_setting('session_authorization');
 (1 row)

 rollback;
+-- test that function option SET ROLE works in parallel workers.
+create role regress_parallel_worker;
+create function set_and_report_role() returns text as
+  $$ select current_setting('role') $$ language sql parallel safe
+  set role = regress_parallel_worker;
+create function set_role_and_error(int) returns int as
+  $$ select 1 / $1 $$ language sql parallel safe
+  set role = regress_parallel_worker;
+set debug_parallel_query = 0;
+select set_and_report_role();
+   set_and_report_role
+-------------------------
+ regress_parallel_worker
+(1 row)
+
+select set_role_and_error(0);
+ERROR:  division by zero
+CONTEXT:  SQL function "set_role_and_error" statement 1
+set debug_parallel_query = 1;
+select set_and_report_role();
+   set_and_report_role
+-------------------------
+ regress_parallel_worker
+(1 row)
+
+select set_role_and_error(0);
+ERROR:  division by zero
+CONTEXT:  SQL function "set_role_and_error" statement 1
+parallel worker
+reset debug_parallel_query;
+drop function set_and_report_role();
+drop function set_role_and_error(int);
+drop role regress_parallel_worker;
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 1a0e7f144d..22384b5ad8 100644
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -520,3 +520,26 @@ select current_setting('session_authorization');
 set debug_parallel_query = 1;
 select current_setting('session_authorization');
 rollback;
+
+-- test that function option SET ROLE works in parallel workers.
+create role regress_parallel_worker;
+
+create function set_and_report_role() returns text as
+  $$ select current_setting('role') $$ language sql parallel safe
+  set role = regress_parallel_worker;
+
+create function set_role_and_error(int) returns int as
+  $$ select 1 / $1 $$ language sql parallel safe
+  set role = regress_parallel_worker;
+
+set debug_parallel_query = 0;
+select set_and_report_role();
+select set_role_and_error(0);
+set debug_parallel_query = 1;
+select set_and_report_role();
+select set_role_and_error(0);
+reset debug_parallel_query;
+
+drop function set_and_report_role();
+drop function set_role_and_error(int);
+drop role regress_parallel_worker;