pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
From: Daniel Verite <daniel@manitou-mail.org>
To: pgsql-hackers@postgresql.org
Subject: Improvements in psql hooks for variables
Date: Fri, 16 Sep 2016 14:35:41 +0200
Message-ID: <7356e741-fa59-4146-a8eb-cf95fd6b21fb@mm> (raw)
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>

 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

Attachments:

  [text/plain] psql-var-hooks-v1.patch (9.4K, ../7356e741-fa59-4146-a8eb-cf95fd6b21fb@mm/2-psql-var-hooks-v1.patch)
  download | inline diff:
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,


view thread (54+ messages)  latest in thread

Message-ID: <7356e741-fa59-4146-a8eb-cf95fd6b21fb@mm>
Permalink:  ../7356e741-fa59-4146-a8eb-cf95fd6b21fb@mm/
Also on:    postgresql.org/message-id/7356e741-fa59-4146-a8eb-cf95fd6b21fb@mm

 ·  · 

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: daniel@manitou-mail.org
  Subject: Re: Improvements in psql hooks for variables
  In-Reply-To: <7356e741-fa59-4146-a8eb-cf95fd6b21fb@mm>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox