From 511ae628f31b4e791cd5c7836e46cb84dcf145fd Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 30 Jan 2017 16:37:15 -0500 Subject: [PATCH] Make psql reject attempts to set special variables to invalid values. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 | 33 +++-- src/bin/psql/command.c | 82 ++++++----- src/bin/psql/common.c | 1 - src/bin/psql/input.c | 2 +- src/bin/psql/mainloop.c | 2 +- src/bin/psql/startup.c | 121 ++++++++------- src/bin/psql/variables.c | 228 +++++++++++++++++++++-------- src/bin/psql/variables.h | 50 +++++-- src/test/regress/expected/psql.out | 11 +- src/test/regress/sql/psql.sql | 8 + 10 files changed, 356 insertions(+), 182 deletions(-) diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 640fe12bbf..4e51e90906 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -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. + + + + diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 0c164a339c..f17f610717 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -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; } diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index e1b04de013..6e3acdc416 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -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; diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c index 972bea4cbf..3e3e97ad0d 100644 --- a/src/bin/psql/input.c +++ b/src/bin/psql/input.c @@ -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; diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c index bb306a4327..dc25b4babc 100644 --- a/src/bin/psql/mainloop.c +++ b/src/bin/psql/mainloop.c @@ -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); diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c index 85aac4a165..0574b5bdfb 100644 --- a/src/bin/psql/startup.c +++ b/src/bin/psql/startup.c @@ -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; } diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c index 2669572aa7..91e4ae8095 100644 --- a/src/bin/psql/variables.c +++ b/src/bin/psql/variables.c @@ -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); +} diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.h index d235b1798e..274b4af553 100644 --- a/src/bin/psql/variables.h +++ b/src/bin/psql/variables.h @@ -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 */ diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out index 464436ab3b..420825aa56 100644 --- a/src/test/regress/expected/psql.out +++ b/src/test/regress/expected/psql.out @@ -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 diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql index 900aa7ee1e..79624b9193 100644 --- a/src/test/regress/sql/psql.sql +++ b/src/test/regress/sql/psql.sql @@ -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_ -- 2.40.0