Invent PGC_SU_BACKEND and mark log_connections/log_disconnections that way.

This new GUC context option allows GUC parameters to have the combined
properties of PGC_BACKEND and PGC_SUSET, ie, they don't change after
session start and non-superusers can't change them.  This is a more
appropriate choice for log_connections and log_disconnections than their
previous context of PGC_BACKEND, because we don't want non-superusers
to be able to affect whether their sessions get logged.

Note: the behavior for log_connections is still a bit odd, in that when
a superuser attempts to set it from PGOPTIONS, the setting takes effect
but it's too late to enable or suppress connection startup logging.
It's debatable whether that's worth fixing, and in any case there is
a reasonable argument for PGC_SU_BACKEND to exist.

In passing, re-pgindent the files touched by this commit.

Fujii Masao, reviewed by Joe Conway and Amit Kapila

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 74110f077226a0546ef84aca7c0f245f0ea7975e..5be8fdcc252de5cea9bf551785a7ef58075d61b8 100644 (file)
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4345,8 +4345,9 @@ local0.*    /var/log/postgresql
        <para>
         Causes each attempted connection to the server to be logged,
         as well as successful completion of client authentication.
-        This parameter cannot be changed after session start.
-        The default is off.
+        Only superusers can change this parameter at session start,
+        and it cannot be changed at all within a session.
+        The default is <literal>off</>.
        </para>
 
        <note>
@@ -4368,11 +4369,12 @@ local0.*    /var/log/postgresql
       </term>
       <listitem>
        <para>
-        This outputs a line in the server log similar to
-        <varname>log_connections</varname> but at session termination,
-        and includes the duration of the session.  This is off by
-        default.
-        This parameter cannot be changed after session start.
+        Causes session terminations to be logged.  The log output
+        provides information similar to <varname>log_connections</varname>,
+        plus the duration of the session.
+        Only superusers can change this parameter at session start,
+        and it cannot be changed at all within a session.
+        The default is <literal>off</>.
        </para>
       </listitem>
      </varlistentry>

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 7b5480faf866c7a44e359f4b5ad870743cc6faac..61f17bf1e9b2468d14f87398203c1cb1d8dcc52a 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3258,7 +3258,7 @@ get_stats_option_name(const char *arg)
  * argv[0] is ignored in either case (it's assumed to be the program name).
  *
  * ctx is PGC_POSTMASTER for secure options, PGC_BACKEND for insecure options
- * coming from the client, or PGC_SUSET for insecure options coming from
+ * coming from the client, or PGC_SU_BACKEND for insecure options coming from
  * a superuser client.
  *
  * If a database name is present in the command line arguments, it's

diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index f5a6a671349d8b5556af0b7bfd71aacea755099c..6a6a4453cd0b552f54fd28880cc71d37e8211ce3 100644 (file)
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -425,7 +425,7 @@ pg_split_opts(char **argv, int *argcp, char *optstr)
 
    while (*optstr)
    {
-       bool last_was_escape = false;
+       bool        last_was_escape = false;
 
        resetStringInfo(&s);
 
@@ -982,7 +982,7 @@ process_startup_options(Port *port, bool am_superuser)
    GucContext  gucctx;
    ListCell   *gucopts;
 
-   gucctx = am_superuser ? PGC_SUSET : PGC_BACKEND;
+   gucctx = am_superuser ? PGC_SU_BACKEND : PGC_BACKEND;
 
    /*
     * First process any command-line switches that were included in the

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0192def17a5b202ec49510a7f5b5b7e1b6d3d57a..b87bfb3ff00c236e59cbf3a69d88f9ff4a7bbe3b 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -493,7 +493,7 @@ static bool data_checksums;
 static int wal_segment_size;
 static bool integer_datetimes;
 static int effective_io_concurrency;
-static bool    assert_enabled;
+static bool assert_enabled;
 
 /* should be static, but commands/variable.c needs to get at this */
 char      *role_string;
@@ -509,6 +509,7 @@ const char *const GucContext_Names[] =
     /* PGC_INTERNAL */ "internal",
     /* PGC_POSTMASTER */ "postmaster",
     /* PGC_SIGHUP */ "sighup",
+    /* PGC_SU_BACKEND */ "superuser-backend",
     /* PGC_BACKEND */ "backend",
     /* PGC_SUSET */ "superuser",
     /* PGC_USERSET */ "user"
@@ -907,7 +908,7 @@ static struct config_bool ConfigureNamesBool[] =
        NULL, NULL, NULL
    },
    {
-       {"log_connections", PGC_BACKEND, LOGGING_WHAT,
+       {"log_connections", PGC_SU_BACKEND, LOGGING_WHAT,
            gettext_noop("Logs each successful connection."),
            NULL
        },
@@ -916,7 +917,7 @@ static struct config_bool ConfigureNamesBool[] =
        NULL, NULL, NULL
    },
    {
-       {"log_disconnections", PGC_BACKEND, LOGGING_WHAT,
+       {"log_disconnections", PGC_SU_BACKEND, LOGGING_WHAT,
            gettext_noop("Logs end of a session, including duration."),
            NULL
        },
@@ -4389,10 +4390,10 @@ SelectConfigFiles(const char *userDoption, const char *progname)
    SetConfigOption("data_directory", DataDir, PGC_POSTMASTER, PGC_S_OVERRIDE);
 
    /*
-    * Now read the config file a second time, allowing any settings in
-    * the PG_AUTOCONF_FILENAME file to take effect.  (This is pretty ugly,
-    * but since we have to determine the DataDir before we can find the
-    * autoconf file, the alternatives seem worse.)
+    * Now read the config file a second time, allowing any settings in the
+    * PG_AUTOCONF_FILENAME file to take effect.  (This is pretty ugly, but
+    * since we have to determine the DataDir before we can find the autoconf
+    * file, the alternatives seem worse.)
     */
    ProcessConfigFile(PGC_POSTMASTER);
 
@@ -5694,16 +5695,27 @@ set_config_option(const char *name, const char *value,
             * signals to individual backends only.
             */
            break;
+       case PGC_SU_BACKEND:
+           /* Reject if we're connecting but user is not superuser */
+           if (context == PGC_BACKEND)
+           {
+               ereport(elevel,
+                       (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                        errmsg("permission denied to set parameter \"%s\"",
+                               name)));
+               return 0;
+           }
+           /* FALL THRU to process the same as PGC_BACKEND */
        case PGC_BACKEND:
            if (context == PGC_SIGHUP)
            {
                /*
-                * If a PGC_BACKEND parameter is changed in the config file,
-                * we want to accept the new value in the postmaster (whence
-                * it will propagate to subsequently-started backends), but
-                * ignore it in existing backends.  This is a tad klugy, but
-                * necessary because we don't re-read the config file during
-                * backend start.
+                * If a PGC_BACKEND or PGC_SU_BACKEND parameter is changed in
+                * the config file, we want to accept the new value in the
+                * postmaster (whence it will propagate to
+                * subsequently-started backends), but ignore it in existing
+                * backends.  This is a tad klugy, but necessary because we
+                * don't re-read the config file during backend start.
                 *
                 * In EXEC_BACKEND builds, this works differently: we load all
                 * nondefault settings from the CONFIG_EXEC_PARAMS file during
@@ -5722,7 +5734,9 @@ set_config_option(const char *name, const char *value,
                    return -1;
 #endif
            }
-           else if (context != PGC_POSTMASTER && context != PGC_BACKEND &&
+           else if (context != PGC_POSTMASTER &&
+                    context != PGC_BACKEND &&
+                    context != PGC_SU_BACKEND &&
                     source != PGC_S_CLIENT)
            {
                ereport(elevel,
@@ -6771,7 +6785,8 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
        if (record == NULL)
            ereport(ERROR,
                    (errcode(ERRCODE_UNDEFINED_OBJECT),
-                  errmsg("unrecognized configuration parameter \"%s\"", name)));
+                    errmsg("unrecognized configuration parameter \"%s\"",
+                           name)));
 
        /*
         * Don't allow the parameters which can't be set in configuration
@@ -6780,16 +6795,17 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
        if ((record->context == PGC_INTERNAL) ||
            (record->flags & GUC_DISALLOW_IN_FILE) ||
            (record->flags & GUC_DISALLOW_IN_AUTO_FILE))
-            ereport(ERROR,
-                    (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
-                     errmsg("parameter \"%s\" cannot be changed",
-                            name)));
+           ereport(ERROR,
+                   (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
+                    errmsg("parameter \"%s\" cannot be changed",
+                           name)));
 
        if (!validate_conf_option(record, name, value, PGC_S_FILE,
                                  ERROR, true, NULL,
                                  &newextra))
            ereport(ERROR,
-           (errmsg("invalid value for parameter \"%s\": \"%s\"", name, value)));
+                   (errmsg("invalid value for parameter \"%s\": \"%s\"",
+                           name, value)));
    }
 
 
@@ -6817,7 +6833,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
    if (Tmpfd < 0)
        ereport(ERROR,
                (errcode_for_file_access(),
-                errmsg("failed to open auto conf temp file \"%s\": %m ",
+                errmsg("failed to open auto conf temp file \"%s\": %m",
                        AutoConfTmpFileName)));
 
    PG_TRY();
@@ -6835,8 +6851,8 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
                infile = AllocateFile(AutoConfFileName, "r");
                if (infile == NULL)
                    ereport(ERROR,
-                           (errmsg("failed to open auto conf file \"%s\": %m ",
-                                   AutoConfFileName)));
+                         (errmsg("failed to open auto conf file \"%s\": %m",
+                                 AutoConfFileName)));
 
                /* parse it */
                ParseConfigFp(infile, AutoConfFileName, 0, LOG, &head, &tail);
@@ -8388,8 +8404,8 @@ read_nondefault_variables(void)
    GucContext  varscontext;
 
    /*
-    * Assert that PGC_BACKEND case in set_config_option() will do the right
-    * thing.
+    * Assert that PGC_BACKEND/PGC_SU_BACKEND case in set_config_option() will
+    * do the right thing.
     */
    Assert(IsInitProcessingMode());
 

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 0a729c11902aae80e17735399daf2e726bd41ede..66b5cd36c59b417596afadc772670e9df03cdd04 100644 (file)
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -36,15 +36,17 @@
  * certain point in their main loop. It's safer to wait than to read a
  * file asynchronously.)
  *
- * BACKEND options can only be set at postmaster startup, from the
- * configuration file, or by client request in the connection startup
- * packet (e.g., from libpq's PGOPTIONS variable).  Furthermore, an
- * already-started backend will ignore changes to such an option in the
- * configuration file.  The idea is that these options are fixed for a
- * given backend once it's started, but they can vary across backends.
+ * BACKEND and SU_BACKEND options can only be set at postmaster startup,
+ * from the configuration file, or by client request in the connection
+ * startup packet (e.g., from libpq's PGOPTIONS variable).  SU_BACKEND
+ * options can be set from the startup packet only when the user is a
+ * superuser.  Furthermore, an already-started backend will ignore changes
+ * to such an option in the configuration file.  The idea is that these
+ * options are fixed for a given backend once it's started, but they can
+ * vary across backends.
  *
  * SUSET options can be set at postmaster startup, with the SIGHUP
- * mechanism, or from SQL if you're a superuser.
+ * mechanism, or from the startup packet or SQL if you're a superuser.
  *
  * USERSET options can be set by anyone any time.
  */
@@ -53,6 +55,7 @@ typedef enum
    PGC_INTERNAL,
    PGC_POSTMASTER,
    PGC_SIGHUP,
+   PGC_SU_BACKEND,
    PGC_BACKEND,
    PGC_SUSET,
    PGC_USERSET
@@ -195,7 +198,8 @@ typedef enum
 #define GUC_UNIT_TIME          0x7000  /* mask for MS, S, MIN */
 
 #define GUC_NOT_WHILE_SEC_REST 0x8000  /* can't set if security restricted */
-#define GUC_DISALLOW_IN_AUTO_FILE  0x00010000  /* can't set in PG_AUTOCONF_FILENAME */
+#define GUC_DISALLOW_IN_AUTO_FILE  0x00010000  /* can't set in
+                                                * PG_AUTOCONF_FILENAME */
 
 /* GUC vars that are actually declared in guc.c, rather than elsewhere */
 extern bool log_duration;