Hi,
Following the discussion on forbidding an AUTOCOMMIT off->on
switch mid-transaction [1], attached is a patch that let the hooks
return a boolean indicating whether a change is allowed.
Using the hooks, bogus assignments to built-in variables can
be dealt with more strictly.
For example, pre-patch behavior:
=# \set ECHO errors
=# \set ECHO on
unrecognized value "on" for "ECHO"; assuming "none"
=# \echo :ECHO
on
which has two problems:
- we have to assume a value, even though we can't know what the user meant.
- after assignment, the user-visible value of the variable diverges from its
internal counterpart (pset.echo in this case).
Post-patch:
=# \set ECHO errors
=# \set ECHO on
unrecognized value "on" for "ECHO"
\set: error while setting variable
=# \echo :ECHO
errors
Both the internal pset.* state and the user-visible value are kept unchanged
is the input value is incorrect.
Concerning AUTOCOMMIT, autocommit_hook() could return false to forbid
a switch when the conditions are not met.
Another user-visible effect of the patch is that, using a bogus value
for a built-in variable on the command-line becomes a fatal error
that prevents psql to continue.
This is not directly intended by the patch but is a consequence
of SetVariable() failing.
Example:
$ ./psql -vECHO=bogus
unrecognized value "bogus" for "ECHO"
psql: could not set variable "ECHO"
$ echo $?
1
The built-in vars concerned by the change are:
booleans: AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP
non-booleans: ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
HISTCONTROL, VERBOSITY, SHOW_CONTEXT
We could go further to close the gap between pset.* and the built-in
variables,
by changing how they're initialized and forbidding deletion as Tom
suggests in [2], but if there's negative feedback on the above changes,
I think we should hear it first.
[1]
https://www.postgresql.org/message-id/f2cb5838-0ee9-4fe3-acc0-df77aeb7d4c7%40mm
[2]
https://www.postgresql.org/message-id/4695.1473961140%40sss.pgh.pa.us
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 7ce05fb..9dcfc0a 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -786,43 +786,59 @@ showVersion(void)
* This isn't an amazingly good place for them, but neither is anywhere else.
*/
-static void
+/*
+ * Hook to set an internal flag from a user-supplied string value.
+ * If the syntax is correct, affect *flag and return true.
+ * Otherwise, keep *flag untouched and return false.
+ */
+static bool
+generic_boolean_hook(const char *newval, const char* varname, bool *flag)
+{
+ bool isvalid;
+ bool val = ParseVariableBool(newval, varname, &isvalid);
+ if (isvalid)
+ *flag = val;
+ return isvalid;
+}
+
+static bool
autocommit_hook(const char *newval)
{
- pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
+ return generic_boolean_hook(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 generic_boolean_hook(newval, "ON_ERROR_STOP",
&pset.on_error_stop);
}
-static void
+static bool
quiet_hook(const char *newval)
{
- pset.quiet = ParseVariableBool(newval, "QUIET");
+ return generic_boolean_hook(newval, "QUIET", &pset.quiet);
}
-static void
+static bool
singleline_hook(const char *newval)
{
- pset.singleline = ParseVariableBool(newval, "SINGLELINE");
+ return generic_boolean_hook(newval, "SINGLELINE", &pset.singleline);
}
-static void
+static bool
singlestep_hook(const char *newval)
{
- pset.singlestep = ParseVariableBool(newval, "SINGLESTEP");
+ return generic_boolean_hook(newval, "SINGLESTEP", &pset.singlestep);
}
-static void
+static bool
fetch_count_hook(const char *newval)
{
pset.fetch_count = ParseVariableNum(newval, -1, -1, false);
+ return true;
}
-static void
+static bool
echo_hook(const char *newval)
{
if (newval == NULL)
@@ -837,39 +853,52 @@ 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;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "ECHO");
+ 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 isvalid;
+ bool val = ParseVariableBool(newval, "ECHO_HIDDEN", &isvalid);
+ if (!isvalid)
+ return false; /* ParseVariableBool printed msg */
+ pset.echo_hidden = val ? PSQL_ECHO_HIDDEN_ON :
PSQL_ECHO_HIDDEN_OFF;
+ }
+ 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 isvalid;
+ bool val = ParseVariableBool(newval, "ON_ERROR_ROLLBACK",
&isvalid);
+ if (isvalid)
+ pset.on_error_rollback = val ? PSQL_ERROR_ROLLBACK_ON :
PSQL_ERROR_ROLLBACK_OFF;
+ else
+ /* ParseVariableBool printed msg if needed */
+ return false;
+ }
+ return true;
}
-static void
+static bool
comp_keyword_case_hook(const char *newval)
{
if (newval == NULL)
@@ -884,13 +913,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;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "COMP_KEYWORD_CASE");
+ return false;
}
+ return true;
}
-static void
+static bool
histcontrol_hook(const char *newval)
{
if (newval == NULL)
@@ -905,31 +935,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;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "HISTCONTROL");
+ 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 +976,17 @@ 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;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "VERBOSITY");
+ 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 +999,14 @@ 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;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "SHOW_CONTEXT");
+ 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 f43f418..4f817c3 100644
--- a/src/bin/psql/variables.c
+++ b/src/bin/psql/variables.c
@@ -86,12 +86,16 @@ GetVariable(VariableSpace space, const char *name)
*
* "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.
+ *
+ * "*valid" reports whether "value" was syntactically valid, unless valid ==
NULL
*/
bool
-ParseVariableBool(const char *value, const char *name)
+ParseVariableBool(const char *value, const char *name, bool *valid)
{
size_t len;
+ if (valid)
+ *valid = true;
if (value == NULL)
return false; /* not set -> assume "off" */
@@ -116,10 +120,16 @@ ParseVariableBool(const char *value, const char *name)
return false;
else
{
- /* NULL is treated as false, so a non-matching value is 'true'
*/
+ /*
+ * NULL is treated as false, so a non-matching value is 'true'.
+ * A caller that cares about syntactic conformance should
+ * check *valid to know whether the value was recognized.
+ */
if (name)
- psql_error("unrecognized value \"%s\" for \"%s\";
assuming \"%s\"\n",
- value, name, "on");
+ psql_error("unrecognized value \"%s\" for \"%s\":
boolean expected\n",
+ value, name);
+ if (*valid)
+ *valid = false;
return true;
}
}
@@ -205,13 +215,19 @@ 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 a hook returns false
*/
+ bool confirmed = true;
if (current->assign_hook)
- (*current->assign_hook) (current->value);
- return true;
+ {
+ confirmed = (*current->assign_hook) (value);
+ }
+ if (confirmed)
+ {
+ if (current->value)
+ free(current->value);
+ current->value = pg_strdup(value);
+ }
+ return confirmed;
}
}
@@ -248,7 +264,7 @@ SetVariableAssignHook(VariableSpace space, const char
*name, VariableAssignHook
{
/* found entry, so update */
current->assign_hook = hook;
- (*hook) (current->value);
+ (void)(*hook) (current->value); /* ignore return value
*/
return true;
}
}
@@ -260,7 +276,7 @@ SetVariableAssignHook(VariableSpace space, const char
*name, VariableAssignHook
current->assign_hook = hook;
current->next = NULL;
previous->next = current;
- (*hook) (NULL);
+ (void)(*hook) (NULL); /* ignore return value */
return true;
}
diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.h
index d7a05a1..9836fc5 100644
--- a/src/bin/psql/variables.h
+++ b/src/bin/psql/variables.h
@@ -20,7 +20,7 @@
* 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,7 +35,7 @@ typedef struct _variable *VariableSpace;
VariableSpace CreateVariableSpace(void);
const char *GetVariable(VariableSpace space, const char *name);
-bool ParseVariableBool(const char *value, const char *name);
+bool ParseVariableBool(const char *value, const char *name, bool
*valid);
int ParseVariableNum(const char *val,
int defaultval,
int faultval,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
