Tom Lane wrote:
> I took a quick look through this. It seems to be going in generally
> the right direction, but here's a couple of thoughts:
Here's an update with these changes:
per Tom's suggestions upthread:
- change ParseVariableBool() signature to return validity as bool.
- remove ParseCheckVariableNum() in favor of using tightened up
ParseVariableNum() and GetVariableNum().
- updated header comments in variables.h
other changes:
- autocommit_hook rejects transitions from OFF to ON when inside a
transaction, per suggestion of Rahila Syed (which was the original
motivation for the set of changes of this patch).
- slight doc update for HISTCONTROL (values outside of enum not longer
allowed)
- add enum-style suggestions on invalid input for \pset x, \pset pager,
and \set of ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
HISTCONTROL, VERBOSITY, SHOW_CONTEXT, \x, \pager
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 9915731..042d277 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3213,9 +3213,8 @@ bar
list. If set to a value of <literal>ignoredups</literal>, lines
matching the previous history line are not entered. A value of
<literal>ignoreboth</literal> combines the two options. If
- unset, or if set to <literal>none</literal> (or any other value
- than those above), all lines read in interactive mode are
- saved on the history list.
+ unset, or if set to <literal>none</literal>, all lines read
+ in interactive mode are saved on the history list.
</para>
<note>
<para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 4139b77..091a138 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -253,26 +253,31 @@ exec_command(const char *cmd,
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;
-
- free(opt1);
- opt1 = read_connect_arg(scan_state);
+ bool on_off;
+ success = ParseVariableBool(opt1 + sizeof(prefix) - 1,
prefix, &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) /* do not attempt to connect if
reuse_previous argument 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 */
@@ -1548,7 +1553,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)
@@ -2660,7 +2665,16 @@ 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;
}
@@ -2669,7 +2683,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;
}
@@ -2724,7 +2738,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;
}
@@ -2756,10 +2770,13 @@ 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;
@@ -2778,7 +2795,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/input.c b/src/bin/psql/input.c
index 972bea4..3e3e97a 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 bb306a4..dc25b4b 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 85aac4a..0b4f43e 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -786,43 +786,71 @@ 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");
+ bool autocommit;
+ if (ParseVariableBool(newval, "AUTOCOMMIT", &autocommit))
+ {
+ /*
+ * Switching AUTOCOMMIT from OFF to ON is not allowed when
inside a
+ * transaction, because it cannot be effective until the current
+ * transaction is ended.
+ */
+ if (autocommit && !pset.autocommit &&
+ pset.db && PQtransactionStatus(pset.db) ==
PQTRANS_INTRANS)
+ {
+ psql_error("cannot set AUTOCOMMIT to %s when inside a
transaction\n", newval);
+ }
+ else
+ {
+ pset.autocommit = autocommit;
+ return true;
+ }
+ }
+ return false;
}
-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, &pset.fetch_count))
+ {
+ psql_error("Invalid value \"%s\" for \"%s\"\nAn integer is
expected\n",
+ newval, "FETCH_COUNT");
+ return false;
+ }
+ return true;
}
-static void
+static bool
echo_hook(const char *newval)
{
if (newval == NULL)
@@ -837,39 +865,55 @@ 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 +928,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 +950,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 +991,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 +1013,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 2669572..d0de198 100644
--- a/src/bin/psql/variables.c
+++ b/src/bin/psql/variables.c
@@ -79,92 +79,103 @@ GetVariable(VariableSpace space, const char *name)
}
/*
- * Try to interpret "value" as boolean value.
+ * Try to interpret "value" as boolean value, and if successful,
+ * put 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'
*/
+ /*
+ * The string is not recognized. We don't clobber *result in
this case.
+ */
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.
+ * Parse a numeric variable from its string representation.
+ * If the syntax is valid, return true and set *result to the value.
+ * Otherwise return false and leave *result unchanged.
*/
-int
-ParseVariableNum(const char *val,
- int defaultval,
- int faultval,
- bool allowtrail)
+bool
+ParseVariableNum(const char *val, int *result)
{
- int result;
+ char *end;
+ int numval;
- if (!val)
- result = defaultval;
- else if (!val[0])
- result = faultval;
- else
+ if (!val || !val[0])
+ return false;
+ errno = 0;
+ numval = strtol(val, &end, 0);
+ if (errno == 0 && *end == '\0')
{
- char *end;
-
- result = strtol(val, &end, 0);
- if (!allowtrail && *end)
- result = faultval;
+ *result = numval;
+ return true;
}
-
- return result;
+ return false;
}
+
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, &result))
+ return result;
+ else
+ return faultval;
}
void
@@ -205,13 +216,26 @@ 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);
- if (current->assign_hook)
- (*current->assign_hook) (current->value);
- return true;
+ /*
+ * Found entry, so update, unless a hook returns false.
+ * The hook needs the value in a buffer with the same
lifespan
+ * as the variable, so allocate it right away, even if
it needs
+ * to be freed just after when the hook returns false.
+ */
+ char *new_value = pg_strdup(value);
+
+ bool confirmed = current->assign_hook ?
+ (*current->assign_hook)
(new_value) : true;
+
+ if (confirmed)
+ {
+ pg_free(current->value);
+ current->value = new_value;
+ }
+ else
+ pg_free(new_value); /* and current->value is
left unchanged */
+
+ return confirmed;
}
}
@@ -248,8 +272,7 @@ SetVariableAssignHook(VariableSpace space, const char
*name, VariableAssignHook
{
/* found entry, so update */
current->assign_hook = hook;
- (*hook) (current->value);
- return true;
+ return (*hook) (current->value);
}
}
@@ -260,8 +283,7 @@ SetVariableAssignHook(VariableSpace space, const char
*name, VariableAssignHook
current->assign_hook = hook;
current->next = NULL;
previous->next = current;
- (*hook) (NULL);
- return true;
+ return (*hook) (NULL);
}
bool
@@ -303,3 +325,14 @@ DeleteVariable(VariableSpace space, const char *name)
return true;
}
+
+/*
+ * Emit error with suggestions for variables or commands
+ * accepting enum-style arguments.
+ */
+void
+PsqlVarEnumError(const char* name, const char* value, const char* suggestions)
+{
+ psql_error("Unrecognized value \"%s\" for \"%s\"\nAvailable values:
%s\n",
+ value, name, suggestions);
+}
diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.h
index d235b17..c7d99d1 100644
--- a/src/bin/psql/variables.h
+++ b/src/bin/psql/variables.h
@@ -12,15 +12,19 @@
* 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.
+ * "assign hook" function that is called before the variable's value is
+ * changed, and that returns a boolean indicating whether the assignment
+ * is confirmed or must be cancelled.
*
- * An "unset" operation causes the hook to be called with newval == NULL.
+ * The hook is called with newval == NULL when a variable is created
+ * along with it by SetVariableAssignHook().
+ * An "unset" operation also causes the hook to be called with newval == NULL,
+ * and is not cancellable even if the hook returns false.
*
* 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);
+typedef bool (*VariableAssignHook) (const char *newval);
struct _variable
{
@@ -35,16 +39,15 @@ 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 *val, int* result);
+
int GetVariableNum(VariableSpace space,
const char *name,
int defaultval,
- int faultval,
- bool allowtrail);
+ int faultval);
+
void PrintVariables(VariableSpace space);
@@ -53,4 +56,6 @@ bool SetVariableAssignHook(VariableSpace space,
const char *name, VariableAssig
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 */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
