Make psql reject attempts to set special variables to invalid values.
authorTom Lane
Mon, 30 Jan 2017 21:37:15 +0000 (16:37 -0500)
committerTom Lane
Mon, 30 Jan 2017 21:37:26 +0000 (16:37 -0500)
Previously, if the user set a special variable such as ECHO to an
unrecognized value, psql would bleat but store the new value anyway, and
then fall back to a default setting for the behavior controlled by the
variable.  This was agreed to be a not particularly good idea.  With
this patch, invalid values result in an error message and no change in
state.

(But this applies only to variables that affect psql's behavior; purely
informational variables such as ENCODING can still be set to random
values.)

To do this, modify the API for psql's assign-hook functions so that they
can return an OK/not OK result, and give them the responsibility for
printing error messages when they reject a value.  Adjust the APIs for
ParseVariableBool and ParseVariableNum to support the new behavior
conveniently.

In passing, document the variable VERSION, which had somehow escaped that.
And improve the quite-inadequate commenting in psql/variables.c.

Daniel Vérité, reviewed by Rahila Syed, some further tweaking by me

Discussion: https://postgr.es/m/7356e741-fa59-4146-a8eb-cf95fd6b21fb@mm

doc/src/sgml/ref/psql-ref.sgml
src/bin/psql/command.c
src/bin/psql/common.c
src/bin/psql/input.c
src/bin/psql/mainloop.c
src/bin/psql/startup.c
src/bin/psql/variables.c
src/bin/psql/variables.h
src/test/regress/expected/psql.out
src/test/regress/sql/psql.sql

index 640fe12bbf638dc76f996f5ab26ea3eeae9ad9ba..4e51e90906c558079921f800ffad8ef0acc31e76 100644 (file)
@@ -3078,10 +3078,8 @@ bar
     by psql. They represent certain option
     settings that can be changed at run time by altering the value of
     the variable, or in some cases represent changeable state of
-    psql. Although
-    you can use these variables for other purposes, this is not
-    recommended, as the program behavior might grow really strange
-    really quickly. By convention, all specially treated variables' names
+    psql.
+    By convention, all specially treated variables' names
     consist of all upper-case ASCII letters (and possibly digits and
     underscores). To ensure maximum compatibility in the future, avoid
     using such variable names for your own purposes. A list of all specially
@@ -3170,12 +3168,11 @@ bar
         start-up, use the switch . If set to
         queries,
         psql prints each query to standard output
-        as it is sent to the server. The switch for this is
+        as it is sent to the server. The switch to select this behavior is
         . If set to errors, then only
         failed queries are displayed on standard error output. The switch
-        for this is . If unset, or if set to
-        none (or any other value than those above) then
-        no queries are displayed.
+        for this behavior is . If unset, or if set to
+        none, then no queries are displayed.
         
         
       
@@ -3201,6 +3198,9 @@ bar
         
         
         The current client character set encoding.
+        This is set every time you connect to a database (including
+        program start-up), and when you change the encoding
+        with \encoding, but it can be unset.
         
         
       
@@ -3241,9 +3241,8 @@ bar
          list. If set to a value of ignoredups, lines
          matching the previous history line are not entered. A value of
          ignoreboth combines the two options. If
-         unset, or if set to none (or any other value
-         than those above), all lines read in interactive mode are
-         saved on the history list.
+         unset, or if set to none (the default), all lines
+         read in interactive mode are saved on the history list.
         
         
         
@@ -3312,7 +3311,7 @@ bar
          to an interactive session of psql
          will terminate the application. If set to a numeric value,
          that many EOF characters are ignored before the
-         application terminates.  If the variable is set but has no
+         application terminates.  If the variable is set but not to a
          numeric value, the default is 10.
         
         
@@ -3477,6 +3476,16 @@ bar
         
       
 
+      
+        VERSION
+        
+        
+        This variable is set at program start-up to
+        reflect psql's version.  It can be unset or changed.
+        
+        
+      
+
     
 
    
index 0c164a339c19917105f1fa7a7d7eafc9a5c5f1ea..f17f61071756a61599e6b7ab06703e1d7da83b4c 100644 (file)
@@ -248,31 +248,37 @@ exec_command(const char *cmd,
                   *opt2,
                   *opt3,
                   *opt4;
-       enum trivalue reuse_previous;
+       enum trivalue reuse_previous = TRI_DEFAULT;
 
        opt1 = read_connect_arg(scan_state);
        if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1) == 0)
        {
-           reuse_previous =
-               ParseVariableBool(opt1 + sizeof(prefix) - 1, prefix) ?
-               TRI_YES : TRI_NO;
+           bool        on_off;
 
-           free(opt1);
-           opt1 = read_connect_arg(scan_state);
+           success = ParseVariableBool(opt1 + sizeof(prefix) - 1,
+                                       "-reuse-previous",
+                                       &on_off);
+           if (success)
+           {
+               reuse_previous = on_off ? TRI_YES : TRI_NO;
+               free(opt1);
+               opt1 = read_connect_arg(scan_state);
+           }
        }
-       else
-           reuse_previous = TRI_DEFAULT;
 
-       opt2 = read_connect_arg(scan_state);
-       opt3 = read_connect_arg(scan_state);
-       opt4 = read_connect_arg(scan_state);
+       if (success)            /* give up if reuse_previous was invalid */
+       {
+           opt2 = read_connect_arg(scan_state);
+           opt3 = read_connect_arg(scan_state);
+           opt4 = read_connect_arg(scan_state);
 
-       success = do_connect(reuse_previous, opt1, opt2, opt3, opt4);
+           success = do_connect(reuse_previous, opt1, opt2, opt3, opt4);
 
+           free(opt2);
+           free(opt3);
+           free(opt4);
+       }
        free(opt1);
-       free(opt2);
-       free(opt3);
-       free(opt4);
    }
 
    /* \cd */
@@ -1208,10 +1214,7 @@ exec_command(const char *cmd,
 
            if (result &&
                !SetVariable(pset.vars, opt, result))
-           {
-               psql_error("\\%s: error while setting variable\n", cmd);
                success = false;
-           }
 
            if (result)
                free(result);
@@ -1325,10 +1328,8 @@ exec_command(const char *cmd,
            }
 
            if (!SetVariable(pset.vars, opt0, newval))
-           {
-               psql_error("\\%s: error while setting variable\n", cmd);
                success = false;
-           }
+
            free(newval);
        }
        free(opt0);
@@ -1564,7 +1565,7 @@ exec_command(const char *cmd,
                                                 OT_NORMAL, NULL, false);
 
        if (opt)
-           pset.timing = ParseVariableBool(opt, "\\timing");
+           success = ParseVariableBool(opt, "\\timing", &pset.timing);
        else
            pset.timing = !pset.timing;
        if (!pset.quiet)
@@ -1589,10 +1590,8 @@ exec_command(const char *cmd,
            success = false;
        }
        else if (!SetVariable(pset.vars, opt, NULL))
-       {
-           psql_error("\\%s: error while setting variable\n", cmd);
            success = false;
-       }
+
        free(opt);
    }
 
@@ -2593,7 +2592,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
            psql_error("\\pset: allowed formats are unaligned, aligned, wrapped, html, asciidoc, latex, latex-longtable, troff-ms\n");
            return false;
        }
-
    }
 
    /* set table line style */
@@ -2612,7 +2610,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
            psql_error("\\pset: allowed line styles are ascii, old-ascii, unicode\n");
            return false;
        }
-
    }
 
    /* set unicode border line style */
@@ -2665,7 +2662,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
    {
        if (value)
            popt->topt.border = atoi(value);
-
    }
 
    /* set expanded/vertical mode */
@@ -2676,7 +2672,17 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
        if (value && pg_strcasecmp(value, "auto") == 0)
            popt->topt.expanded = 2;
        else if (value)
-           popt->topt.expanded = ParseVariableBool(value, param);
+       {
+           bool        on_off;
+
+           if (ParseVariableBool(value, NULL, &on_off))
+               popt->topt.expanded = on_off ? 1 : 0;
+           else
+           {
+               PsqlVarEnumError(param, value, "on, off, auto");
+               return false;
+           }
+       }
        else
            popt->topt.expanded = !popt->topt.expanded;
    }
@@ -2685,7 +2691,7 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
    else if (strcmp(param, "numericlocale") == 0)
    {
        if (value)
-           popt->topt.numericLocale = ParseVariableBool(value, param);
+           return ParseVariableBool(value, param, &popt->topt.numericLocale);
        else
            popt->topt.numericLocale = !popt->topt.numericLocale;
    }
@@ -2740,7 +2746,7 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
    else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
    {
        if (value)
-           popt->topt.tuples_only = ParseVariableBool(value, param);
+           return ParseVariableBool(value, param, &popt->topt.tuples_only);
        else
            popt->topt.tuples_only = !popt->topt.tuples_only;
    }
@@ -2772,10 +2778,14 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
            popt->topt.pager = 2;
        else if (value)
        {
-           if (ParseVariableBool(value, param))
-               popt->topt.pager = 1;
-           else
-               popt->topt.pager = 0;
+           bool        on_off;
+
+           if (!ParseVariableBool(value, NULL, &on_off))
+           {
+               PsqlVarEnumError(param, value, "on, off, always");
+               return false;
+           }
+           popt->topt.pager = on_off ? 1 : 0;
        }
        else if (popt->topt.pager == 1)
            popt->topt.pager = 0;
@@ -2794,7 +2804,7 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
    else if (strcmp(param, "footer") == 0)
    {
        if (value)
-           popt->topt.default_footer = ParseVariableBool(value, param);
+           return ParseVariableBool(value, param, &popt->topt.default_footer);
        else
            popt->topt.default_footer = !popt->topt.default_footer;
    }
index e1b04de01321e607f9c1791050945fc124e9b678..6e3acdc416eb2534b98dc2914241da72940d0eb0 100644 (file)
@@ -841,7 +841,6 @@ StoreQueryTuple(const PGresult *result)
 
            if (!SetVariable(pset.vars, varname, value))
            {
-               psql_error("could not set variable \"%s\"\n", varname);
                free(varname);
                success = false;
                break;
index 972bea4cbf8ca993da44e7491e070d269ded1094..3e3e97ad0d22bf5db1671657c1e17de712947a7f 100644 (file)
@@ -541,7 +541,7 @@ finishInput(void)
    {
        int         hist_size;
 
-       hist_size = GetVariableNum(pset.vars, "HISTSIZE", 500, -1, true);
+       hist_size = GetVariableNum(pset.vars, "HISTSIZE", 500, -1);
        (void) saveHistory(psql_history, hist_size);
        free(psql_history);
        psql_history = NULL;
index bb306a432711d771ac735f27913ff4c1c1d52dfa..dc25b4babc5a32f85d457cf082c8bf5382a923e8 100644 (file)
@@ -162,7 +162,7 @@ MainLoop(FILE *source)
                /* This tries to mimic bash's IGNOREEOF feature. */
                count_eof++;
 
-               if (count_eof < GetVariableNum(pset.vars, "IGNOREEOF", 0, 10, false))
+               if (count_eof < GetVariableNum(pset.vars, "IGNOREEOF", 0, 10))
                {
                    if (!pset.quiet)
                        printf(_("Use \"\\q\" to leave %s.\n"), pset.progname);
index 85aac4a1659bedc4f7c5d19fec996991cbef40f2..0574b5bdfb18f01c49afad55e5894d0b8e295dde 100644 (file)
@@ -588,11 +588,7 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts * options)
                    {
                        *equal_loc = '\0';
                        if (!SetVariable(pset.vars, value, equal_loc + 1))
-                       {
-                           fprintf(stderr, _("%s: could not set variable \"%s\"\n"),
-                                   pset.progname, value);
                            exit(EXIT_FAILURE);
-                       }
                    }
 
                    free(value);
@@ -786,43 +782,47 @@ showVersion(void)
  * This isn't an amazingly good place for them, but neither is anywhere else.
  */
 
-static void
+static bool
 autocommit_hook(const char *newval)
 {
-   pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
+   return ParseVariableBool(newval, "AUTOCOMMIT", &pset.autocommit);
 }
 
-static void
+static bool
 on_error_stop_hook(const char *newval)
 {
-   pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
+   return ParseVariableBool(newval, "ON_ERROR_STOP", &pset.on_error_stop);
 }
 
-static void
+static bool
 quiet_hook(const char *newval)
 {
-   pset.quiet = ParseVariableBool(newval, "QUIET");
+   return ParseVariableBool(newval, "QUIET", &pset.quiet);
 }
 
-static void
+static bool
 singleline_hook(const char *newval)
 {
-   pset.singleline = ParseVariableBool(newval, "SINGLELINE");
+   return ParseVariableBool(newval, "SINGLELINE", &pset.singleline);
 }
 
-static void
+static bool
 singlestep_hook(const char *newval)
 {
-   pset.singlestep = ParseVariableBool(newval, "SINGLESTEP");
+   return ParseVariableBool(newval, "SINGLESTEP", &pset.singlestep);
 }
 
-static void
+static bool
 fetch_count_hook(const char *newval)
 {
-   pset.fetch_count = ParseVariableNum(newval, -1, -1, false);
+   if (newval == NULL)
+       pset.fetch_count = -1;  /* default value */
+   else if (!ParseVariableNum(newval, "FETCH_COUNT", &pset.fetch_count))
+       return false;
+   return true;
 }
 
-static void
+static bool
 echo_hook(const char *newval)
 {
    if (newval == NULL)
@@ -837,39 +837,57 @@ echo_hook(const char *newval)
        pset.echo = PSQL_ECHO_NONE;
    else
    {
-       psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-                  newval, "ECHO", "none");
-       pset.echo = PSQL_ECHO_NONE;
+       PsqlVarEnumError("ECHO", newval, "none, errors, queries, all");
+       return false;
    }
+   return true;
 }
 
-static void
+static bool
 echo_hidden_hook(const char *newval)
 {
    if (newval == NULL)
        pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
    else if (pg_strcasecmp(newval, "noexec") == 0)
        pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
-   else if (ParseVariableBool(newval, "ECHO_HIDDEN"))
-       pset.echo_hidden = PSQL_ECHO_HIDDEN_ON;
-   else    /* ParseVariableBool printed msg if needed */
-       pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
+   else
+   {
+       bool        on_off;
+
+       if (ParseVariableBool(newval, NULL, &on_off))
+           pset.echo_hidden = on_off ? PSQL_ECHO_HIDDEN_ON : PSQL_ECHO_HIDDEN_OFF;
+       else
+       {
+           PsqlVarEnumError("ECHO_HIDDEN", newval, "on, off, noexec");
+           return false;
+       }
+   }
+   return true;
 }
 
-static void
+static bool
 on_error_rollback_hook(const char *newval)
 {
    if (newval == NULL)
        pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
    else if (pg_strcasecmp(newval, "interactive") == 0)
        pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE;
-   else if (ParseVariableBool(newval, "ON_ERROR_ROLLBACK"))
-       pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON;
-   else    /* ParseVariableBool printed msg if needed */
-       pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
+   else
+   {
+       bool        on_off;
+
+       if (ParseVariableBool(newval, NULL, &on_off))
+           pset.on_error_rollback = on_off ? PSQL_ERROR_ROLLBACK_ON : PSQL_ERROR_ROLLBACK_OFF;
+       else
+       {
+           PsqlVarEnumError("ON_ERROR_ROLLBACK", newval, "on, off, interactive");
+           return false;
+       }
+   }
+   return true;
 }
 
-static void
+static bool
 comp_keyword_case_hook(const char *newval)
 {
    if (newval == NULL)
@@ -884,13 +902,14 @@ comp_keyword_case_hook(const char *newval)
        pset.comp_case = PSQL_COMP_CASE_LOWER;
    else
    {
-       psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-                  newval, "COMP_KEYWORD_CASE", "preserve-upper");
-       pset.comp_case = PSQL_COMP_CASE_PRESERVE_UPPER;
+       PsqlVarEnumError("COMP_KEYWORD_CASE", newval,
+                        "lower, upper, preserve-lower, preserve-upper");
+       return false;
    }
+   return true;
 }
 
-static void
+static bool
 histcontrol_hook(const char *newval)
 {
    if (newval == NULL)
@@ -905,31 +924,35 @@ histcontrol_hook(const char *newval)
        pset.histcontrol = hctl_none;
    else
    {
-       psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-                  newval, "HISTCONTROL", "none");
-       pset.histcontrol = hctl_none;
+       PsqlVarEnumError("HISTCONTROL", newval,
+                        "none, ignorespace, ignoredups, ignoreboth");
+       return false;
    }
+   return true;
 }
 
-static void
+static bool
 prompt1_hook(const char *newval)
 {
    pset.prompt1 = newval ? newval : "";
+   return true;
 }
 
-static void
+static bool
 prompt2_hook(const char *newval)
 {
    pset.prompt2 = newval ? newval : "";
+   return true;
 }
 
-static void
+static bool
 prompt3_hook(const char *newval)
 {
    pset.prompt3 = newval ? newval : "";
+   return true;
 }
 
-static void
+static bool
 verbosity_hook(const char *newval)
 {
    if (newval == NULL)
@@ -942,16 +965,16 @@ verbosity_hook(const char *newval)
        pset.verbosity = PQERRORS_VERBOSE;
    else
    {
-       psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-                  newval, "VERBOSITY", "default");
-       pset.verbosity = PQERRORS_DEFAULT;
+       PsqlVarEnumError("VERBOSITY", newval, "default, terse, verbose");
+       return false;
    }
 
    if (pset.db)
        PQsetErrorVerbosity(pset.db, pset.verbosity);
+   return true;
 }
 
-static void
+static bool
 show_context_hook(const char *newval)
 {
    if (newval == NULL)
@@ -964,13 +987,13 @@ show_context_hook(const char *newval)
        pset.show_context = PQSHOW_CONTEXT_ALWAYS;
    else
    {
-       psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-                  newval, "SHOW_CONTEXT", "errors");
-       pset.show_context = PQSHOW_CONTEXT_ERRORS;
+       PsqlVarEnumError("SHOW_CONTEXT", newval, "never, errors, always");
+       return false;
    }
 
    if (pset.db)
        PQsetErrorContextVisibility(pset.db, pset.show_context);
+   return true;
 }
 
 
index 2669572aa7e922a896d8136d736808d168b908c2..91e4ae8095302237e7ba5133bc6391b07bd61005 100644 (file)
@@ -58,6 +58,11 @@ CreateVariableSpace(void)
    return ptr;
 }
 
+/*
+ * Get string value of variable, or NULL if it's not defined.
+ *
+ * Note: result is valid until variable is next assigned to.
+ */
 const char *
 GetVariable(VariableSpace space, const char *name)
 {
@@ -79,94 +84,121 @@ GetVariable(VariableSpace space, const char *name)
 }
 
 /*
- * Try to interpret "value" as boolean value.
+ * Try to interpret "value" as a boolean value, and if successful,
+ * store it in *result.  Otherwise don't clobber *result.
  *
  * Valid values are: true, false, yes, no, on, off, 1, 0; as well as unique
  * prefixes thereof.
  *
  * "name" is the name of the variable we're assigning to, to use in error
  * report if any.  Pass name == NULL to suppress the error report.
+ *
+ * Return true when "value" is syntactically valid, false otherwise.
  */
 bool
-ParseVariableBool(const char *value, const char *name)
+ParseVariableBool(const char *value, const char *name, bool *result)
 {
    size_t      len;
+   bool        valid = true;
 
    if (value == NULL)
-       return false;           /* not set -> assume "off" */
+   {
+       *result = false;        /* not set -> assume "off" */
+       return valid;
+   }
 
    len = strlen(value);
 
-   if (pg_strncasecmp(value, "true", len) == 0)
-       return true;
-   else if (pg_strncasecmp(value, "false", len) == 0)
-       return false;
-   else if (pg_strncasecmp(value, "yes", len) == 0)
-       return true;
-   else if (pg_strncasecmp(value, "no", len) == 0)
-       return false;
+   if (len > 0 && pg_strncasecmp(value, "true", len) == 0)
+       *result = true;
+   else if (len > 0 && pg_strncasecmp(value, "false", len) == 0)
+       *result = false;
+   else if (len > 0 && pg_strncasecmp(value, "yes", len) == 0)
+       *result = true;
+   else if (len > 0 && pg_strncasecmp(value, "no", len) == 0)
+       *result = false;
    /* 'o' is not unique enough */
    else if (pg_strncasecmp(value, "on", (len > 2 ? len : 2)) == 0)
-       return true;
+       *result = true;
    else if (pg_strncasecmp(value, "off", (len > 2 ? len : 2)) == 0)
-       return false;
+       *result = false;
    else if (pg_strcasecmp(value, "1") == 0)
-       return true;
+       *result = true;
    else if (pg_strcasecmp(value, "0") == 0)
-       return false;
+       *result = false;
    else
    {
-       /* NULL is treated as false, so a non-matching value is 'true' */
+       /* string is not recognized; don't clobber *result */
        if (name)
-           psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
-                      value, name, "on");
-       return true;
+           psql_error("unrecognized value \"%s\" for \"%s\": boolean expected\n",
+                      value, name);
+       valid = false;
    }
+   return valid;
 }
 
-
 /*
- * Read numeric variable, or defaultval if it is not set, or faultval if its
- * value is not a valid numeric string.  If allowtrail is false, this will
- * include the case where there are trailing characters after the number.
+ * Try to interpret "value" as an integer value, and if successful,
+ * store it in *result.  Otherwise don't clobber *result.
+ *
+ * "name" is the name of the variable we're assigning to, to use in error
+ * report if any.  Pass name == NULL to suppress the error report.
+ *
+ * Return true when "value" is syntactically valid, false otherwise.
  */
-int
-ParseVariableNum(const char *val,
-                int defaultval,
-                int faultval,
-                bool allowtrail)
+bool
+ParseVariableNum(const char *value, const char *name, int *result)
 {
-   int         result;
+   char       *end;
+   long        numval;
 
-   if (!val)
-       result = defaultval;
-   else if (!val[0])
-       result = faultval;
+   if (value == NULL)
+       return false;
+   errno = 0;
+   numval = strtol(value, &end, 0);
+   if (errno == 0 && *end == '\0' && end != value && numval == (int) numval)
+   {
+       *result = (int) numval;
+       return true;
+   }
    else
    {
-       char       *end;
-
-       result = strtol(val, &end, 0);
-       if (!allowtrail && *end)
-           result = faultval;
+       /* string is not recognized; don't clobber *result */
+       if (name)
+           psql_error("invalid value \"%s\" for \"%s\": integer expected\n",
+                      value, name);
+       return false;
    }
-
-   return result;
 }
 
+/*
+ * Read integer value of the numeric variable named "name".
+ *
+ * Return defaultval if it is not set, or faultval if its value is not a
+ * valid integer.  (No error message is issued.)
+ */
 int
 GetVariableNum(VariableSpace space,
               const char *name,
               int defaultval,
-              int faultval,
-              bool allowtrail)
+              int faultval)
 {
    const char *val;
+   int         result;
 
    val = GetVariable(space, name);
-   return ParseVariableNum(val, defaultval, faultval, allowtrail);
+   if (!val)
+       return defaultval;
+
+   if (ParseVariableNum(val, NULL, &result))
+       return result;
+   else
+       return faultval;
 }
 
+/*
+ * Print values of all variables.
+ */
 void
 PrintVariables(VariableSpace space)
 {
@@ -184,17 +216,28 @@ PrintVariables(VariableSpace space)
    }
 }
 
+/*
+ * Set the variable named "name" to value "value",
+ * or delete it if "value" is NULL.
+ *
+ * Returns true if successful, false if not; in the latter case a suitable
+ * error message has been printed, except for the unexpected case of
+ * space or name being NULL.
+ */
 bool
 SetVariable(VariableSpace space, const char *name, const char *value)
 {
    struct _variable *current,
               *previous;
 
-   if (!space)
+   if (!space || !name)
        return false;
 
    if (!valid_variable_name(name))
+   {
+       psql_error("invalid variable name: \"%s\"\n", name);
        return false;
+   }
 
    if (!value)
        return DeleteVariable(space, name);
@@ -205,13 +248,30 @@ SetVariable(VariableSpace space, const char *name, const char *value)
    {
        if (strcmp(current->name, name) == 0)
        {
-           /* found entry, so update */
-           if (current->value)
-               free(current->value);
-           current->value = pg_strdup(value);
+           /*
+            * Found entry, so update, unless hook returns false.  The hook
+            * may need the passed value to have the same lifespan as the
+            * variable, so allocate it right away, even though we'll have to
+            * free it again if the hook returns false.
+            */
+           char       *new_value = pg_strdup(value);
+           bool        confirmed;
+
            if (current->assign_hook)
-               (*current->assign_hook) (current->value);
-           return true;
+               confirmed = (*current->assign_hook) (new_value);
+           else
+               confirmed = true;
+
+           if (confirmed)
+           {
+               if (current->value)
+                   pg_free(current->value);
+               current->value = new_value;
+           }
+           else
+               pg_free(new_value);     /* current->value is left unchanged */
+
+           return confirmed;
        }
    }
 
@@ -226,19 +286,29 @@ SetVariable(VariableSpace space, const char *name, const char *value)
 }
 
 /*
- * This both sets a hook function, and calls it on the current value (if any)
+ * Attach an assign hook function to the named variable.
+ *
+ * If the variable doesn't already exist, create it with value NULL,
+ * just so we have a place to store the hook function.  (Externally,
+ * this isn't different from it not being defined.)
+ *
+ * The hook is immediately called on the variable's current value.  This is
+ * meant to let it update any derived psql state.  If the hook doesn't like
+ * the current value, it will print a message to that effect, but we'll ignore
+ * it.  Generally we do not expect any such failure here, because this should
+ * get called before any user-supplied value is assigned.
  */
-bool
+void
 SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook hook)
 {
    struct _variable *current,
               *previous;
 
-   if (!space)
-       return false;
+   if (!space || !name)
+       return;
 
    if (!valid_variable_name(name))
-       return false;
+       return;
 
    for (previous = space, current = space->next;
         current;
@@ -248,8 +318,8 @@ SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook
        {
            /* found entry, so update */
            current->assign_hook = hook;
-           (*hook) (current->value);
-           return true;
+           (void) (*hook) (current->value);
+           return;
        }
    }
 
@@ -260,16 +330,24 @@ SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook
    current->assign_hook = hook;
    current->next = NULL;
    previous->next = current;
-   (*hook) (NULL);
-   return true;
+   (void) (*hook) (NULL);
 }
 
+/*
+ * Convenience function to set a variable's value to "on".
+ */
 bool
 SetVariableBool(VariableSpace space, const char *name)
 {
    return SetVariable(space, name, "on");
 }
 
+/*
+ * Attempt to delete variable.
+ *
+ * If unsuccessful, print a message and return "false".
+ * Deleting a nonexistent variable is not an error.
+ */
 bool
 DeleteVariable(VariableSpace space, const char *name)
 {
@@ -277,7 +355,7 @@ DeleteVariable(VariableSpace space, const char *name)
               *previous;
 
    if (!space)
-       return false;
+       return true;
 
    for (previous = space, current = space->next;
         current;
@@ -285,14 +363,21 @@ DeleteVariable(VariableSpace space, const char *name)
    {
        if (strcmp(current->name, name) == 0)
        {
-           if (current->value)
-               free(current->value);
-           current->value = NULL;
-           /* Physically delete only if no hook function to remember */
            if (current->assign_hook)
-               (*current->assign_hook) (NULL);
+           {
+               /* Allow deletion only if hook is okay with NULL value */
+               if (!(*current->assign_hook) (NULL))
+                   return false;       /* message printed by hook */
+               if (current->value)
+                   free(current->value);
+               current->value = NULL;
+               /* Don't delete entry, or we'd forget the hook function */
+           }
            else
            {
+               /* We can delete the entry as well as its value */
+               if (current->value)
+                   free(current->value);
                previous->next = current->next;
                free(current->name);
                free(current);
@@ -303,3 +388,16 @@ DeleteVariable(VariableSpace space, const char *name)
 
    return true;
 }
+
+/*
+ * Emit error with suggestions for variables or commands
+ * accepting enum-style arguments.
+ * This function just exists to standardize the wording.
+ * suggestions should follow the format "fee, fi, fo, fum".
+ */
+void
+PsqlVarEnumError(const char *name, const char *value, const char *suggestions)
+{
+   psql_error("unrecognized value \"%s\" for \"%s\"\nAvailable values are: %s.\n",
+              value, name, suggestions);
+}
index d235b1798e44f74aef8d13d55aadd6ffc54fff73..274b4af5537ab27ca6d626a35688d4972b09e961 100644 (file)
@@ -3,25 +3,39 @@
  *
  * Copyright (c) 2000-2017, PostgreSQL Global Development Group
  *
+ * This implements a sort of variable repository.  One could also think of it
+ * as a cheap version of an associative array.  Each variable has a string
+ * name and a string value.  The value can't be NULL, or more precisely
+ * that's not distinguishable from the variable being unset.
+ *
  * src/bin/psql/variables.h
  */
 #ifndef VARIABLES_H
 #define VARIABLES_H
 
 /*
- * This implements a sort of variable repository. One could also think of it
- * as a cheap version of an associative array. In each one of these
- * datastructures you can store name/value pairs.  There can also be an
- * "assign hook" function that is called whenever the variable's value is
- * changed.
+ * Variables can be given "assign hook" functions.  The assign hook can
+ * prevent invalid values from being assigned, and can update internal C
+ * variables to keep them in sync with the variable's current value.
+ *
+ * A hook function is called before any attempted assignment, with the
+ * proposed new value of the variable (or with NULL, if an \unset is being
+ * attempted).  If it returns false, the assignment doesn't occur --- it
+ * should print an error message with psql_error() to tell the user why.
  *
- * An "unset" operation causes the hook to be called with newval == NULL.
+ * When a hook function is installed with SetVariableAssignHook(), it is
+ * called with the variable's current value (or with NULL, if it wasn't set
+ * yet).  But its return value is ignored in this case.  The hook should be
+ * set before any possibly-invalid value can be assigned.
+ */
+typedef bool (*VariableAssignHook) (const char *newval);
+
+/*
+ * Data structure representing one variable.
  *
  * Note: if value == NULL then the variable is logically unset, but we are
  * keeping the struct around so as not to forget about its hook function.
  */
-typedef void (*VariableAssignHook) (const char *newval);
-
 struct _variable
 {
    char       *name;
@@ -30,27 +44,31 @@ struct _variable
    struct _variable *next;
 };
 
+/* Data structure representing a set of variables */
 typedef struct _variable *VariableSpace;
 
+
 VariableSpace CreateVariableSpace(void);
 const char *GetVariable(VariableSpace space, const char *name);
 
-bool       ParseVariableBool(const char *value, const char *name);
-int ParseVariableNum(const char *val,
-                int defaultval,
-                int faultval,
-                bool allowtrail);
+bool ParseVariableBool(const char *value, const char *name,
+                 bool *result);
+
+bool ParseVariableNum(const char *value, const char *name,
+                int *result);
+
 int GetVariableNum(VariableSpace space,
               const char *name,
               int defaultval,
-              int faultval,
-              bool allowtrail);
+              int faultval);
 
 void       PrintVariables(VariableSpace space);
 
 bool       SetVariable(VariableSpace space, const char *name, const char *value);
-bool       SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook hook);
+void       SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook hook);
 bool       SetVariableBool(VariableSpace space, const char *name);
 bool       DeleteVariable(VariableSpace space, const char *name);
 
+void       PsqlVarEnumError(const char *name, const char *value, const char *suggestions);
+
 #endif   /* VARIABLES_H */
index 464436ab3b31031a8d1c48c5f27c3f6875fdca60..420825aa56d9dc9c2bd25203dc33150f008ffc25 100644 (file)
@@ -2,6 +2,15 @@
 -- Tests for psql features that aren't closely connected to any
 -- specific server features
 --
+-- \set
+-- fail: invalid name
+\set invalid/name foo
+invalid variable name: "invalid/name"
+-- fail: invalid value for special variable
+\set AUTOCOMMIT foo
+unrecognized value "foo" for "AUTOCOMMIT": boolean expected
+\set FETCH_COUNT foo
+invalid value "foo" for "FETCH_COUNT": integer expected
 -- \gset
 select 10 as test01, 20 as test02, 'Hello' as test03 \gset pref01_
 \echo :pref01_test01 :pref01_test02 :pref01_test03
@@ -9,7 +18,7 @@ select 10 as test01, 20 as test02, 'Hello' as test03 \gset pref01_
 -- should fail: bad variable name
 select 10 as "bad name"
 \gset
-could not set variable "bad name"
+invalid variable name: "bad name"
 -- multiple backslash commands in one line
 select 1 as x, 2 as y \gset pref01_ \\ \echo :pref01_x
 1
index 900aa7ee1e5fe6791eff7f8e05d1b27fc08b3c13..79624b9193adeea9a48f1308c5efa9668794f482 100644 (file)
@@ -3,6 +3,14 @@
 -- specific server features
 --
 
+-- \set
+
+-- fail: invalid name
+\set invalid/name foo
+-- fail: invalid value for special variable
+\set AUTOCOMMIT foo
+\set FETCH_COUNT foo
+
 -- \gset
 
 select 10 as test01, 20 as test02, 'Hello' as test03 \gset pref01_