From: Tom Lane <tgl@sss.pgh.pa.us>
To: "Daniel Verite" <daniel@manitou-mail.org>
cc: "Rahila Syed" <rahilasyed90@gmail.com>,
        "Stephen Frost" <sfrost@snowman.net>,
        "Ashutosh Bapat" <ashutosh.bapat@enterprisedb.com>,
        "pgsql-hackers" <pgsql-hackers@postgresql.org>,
        "Andrew Dunstan" <andrew@dunslane.net>
Subject: Re: Improvements in psql hooks for variables
In-reply-to: <0451dd1e-aad1-4f9c-855e-6cc6584d4b54@manitou-mail.org>
References: <0451dd1e-aad1-4f9c-855e-6cc6584d4b54@manitou-mail.org>
Comments: In-reply-to "Daniel Verite" <daniel@manitou-mail.org>
	message dated "Tue, 31 Jan 2017 12:57:45 +0100"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <30536.1485881363.0@sss.pgh.pa.us>
Date: Tue, 31 Jan 2017 11:52:13 -0500
Message-ID: <30629.1485881533@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <30536.1485881363.1@sss.pgh.pa.us>

"Daniel Verite" <daniel@manitou-mail.org> writes:
> 	Tom Lane wrote:
>> One possible compromise that would address your concern about display
>> is to modify the hook API some more so that variable hooks could actually
>> substitute new values.  Then for example the bool-variable hooks could
>> effectively replace "\set AUTOCOMMIT" by "\set AUTOCOMMIT on" and
>> "\unset AUTOCOMMIT" by "\set AUTOCOMMIT off", ensuring that interrogation
>> of their values always produces sane results.

> Agreed, that looks like a good compromise.

Attached is a draft patch for that.  I chose to make a second hook rather
than complicate the assign hook API, mainly because it allows more code
sharing --- all the bool vars can share the same substitute hook, and
so can the three-way vars as long as "on" and "off" are the appropriate
substitutes.

I only touched the behavior for bool vars here, but if people like this
solution it could be fleshed out to apply to all the built-in variables.

Maybe we should merge SetVariableSubstituteHook and SetVariableAssignHook
into one function?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="improve-psql-bool-var-behavior-1.patch";
	charset="us-ascii"
Content-ID: <30536.1485881363.2@sss.pgh.pa.us>
Content-Description: improve-psql-bool-var-behavior-1.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 0574b5b..985cfcb 100644
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** showVersion(void)
*** 777,787 ****
=20=20
=20=20
  /*
!  * Assign hooks for psql variables.
   *
   * This isn't an amazingly good place for them, but neither is anywhere e=
lse.
   */
=20=20
  static bool
  autocommit_hook(const char *newval)
  {
--- 777,804 ----
=20=20
=20=20
  /*
!  * Substitute hooks and assign hooks for psql variables.
   *
   * This isn't an amazingly good place for them, but neither is anywhere e=
lse.
   */
=20=20
+ static char *
+ bool_substitute_hook(char *newval)
+ {
+ 	if (newval =3D=3D NULL)
+ 	{
+ 		/* "\unset FOO" becomes "\set FOO off" */
+ 		newval =3D pg_strdup("off");
+ 	}
+ 	else if (newval[0] =3D=3D '\0')
+ 	{
+ 		/* "\set FOO" becomes "\set FOO on" */
+ 		pg_free(newval);
+ 		newval =3D pg_strdup("on");
+ 	}
+ 	return newval;
+ }
+=20
  static bool
  autocommit_hook(const char *newval)
  {
*************** EstablishVariableSpace(void)
*** 1002,1015 ****
--- 1019,1039 ----
  {
  	pset.vars =3D CreateVariableSpace();
=20=20
+ 	SetVariableSubstituteHook(pset.vars, "AUTOCOMMIT", bool_substitute_hook);
  	SetVariableAssignHook(pset.vars, "AUTOCOMMIT", autocommit_hook);
+ 	SetVariableSubstituteHook(pset.vars, "ON_ERROR_STOP", bool_substitute_ho=
ok);
  	SetVariableAssignHook(pset.vars, "ON_ERROR_STOP", on_error_stop_hook);
+ 	SetVariableSubstituteHook(pset.vars, "QUIET", bool_substitute_hook);
  	SetVariableAssignHook(pset.vars, "QUIET", quiet_hook);
+ 	SetVariableSubstituteHook(pset.vars, "SINGLELINE", bool_substitute_hook);
  	SetVariableAssignHook(pset.vars, "SINGLELINE", singleline_hook);
+ 	SetVariableSubstituteHook(pset.vars, "SINGLESTEP", bool_substitute_hook);
  	SetVariableAssignHook(pset.vars, "SINGLESTEP", singlestep_hook);
  	SetVariableAssignHook(pset.vars, "FETCH_COUNT", fetch_count_hook);
  	SetVariableAssignHook(pset.vars, "ECHO", echo_hook);
+ 	SetVariableSubstituteHook(pset.vars, "ECHO_HIDDEN", bool_substitute_hook=
);
  	SetVariableAssignHook(pset.vars, "ECHO_HIDDEN", echo_hidden_hook);
+ 	SetVariableSubstituteHook(pset.vars, "ON_ERROR_ROLLBACK", bool_substitut=
e_hook);
  	SetVariableAssignHook(pset.vars, "ON_ERROR_ROLLBACK", on_error_rollback_=
hook);
  	SetVariableAssignHook(pset.vars, "COMP_KEYWORD_CASE", comp_keyword_case_=
hook);
  	SetVariableAssignHook(pset.vars, "HISTCONTROL", histcontrol_hook);
diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c
index 91e4ae8..61c4ccc 100644
*** a/src/bin/psql/variables.c
--- b/src/bin/psql/variables.c
*************** CreateVariableSpace(void)
*** 52,57 ****
--- 52,58 ----
  	ptr =3D pg_malloc(sizeof *ptr);
  	ptr->name =3D NULL;
  	ptr->value =3D NULL;
+ 	ptr->substitute_hook =3D NULL;
  	ptr->assign_hook =3D NULL;
  	ptr->next =3D NULL;
=20=20
*************** ParseVariableBool(const char *value, con
*** 101,111 ****
  	size_t		len;
  	bool		valid =3D true;
=20=20
  	if (value =3D=3D NULL)
! 	{
! 		*result =3D false;		/* not set -> assume "off" */
! 		return valid;
! 	}
=20=20
  	len =3D strlen(value);
=20=20
--- 102,110 ----
  	size_t		len;
  	bool		valid =3D true;
=20=20
+ 	/* Treat "unset" as an empty string, which will lead to error below */
  	if (value =3D=3D NULL)
! 		value =3D "";
=20=20
  	len =3D strlen(value);
=20=20
*************** ParseVariableNum(const char *value, cons
*** 152,159 ****
  	char	   *end;
  	long		numval;
=20=20
  	if (value =3D=3D NULL)
! 		return false;
  	errno =3D 0;
  	numval =3D strtol(value, &end, 0);
  	if (errno =3D=3D 0 && *end =3D=3D '\0' && end !=3D value && numval =3D=
=3D (int) numval)
--- 151,160 ----
  	char	   *end;
  	long		numval;
=20=20
+ 	/* Treat "unset" as an empty string, which will lead to error below */
  	if (value =3D=3D NULL)
! 		value =3D "";
!=20
  	errno =3D 0;
  	numval =3D strtol(value, &end, 0);
  	if (errno =3D=3D 0 && *end =3D=3D '\0' && end !=3D value && numval =3D=
=3D (int) numval)
*************** SetVariable(VariableSpace space, const c
*** 235,247 ****
=20=20
  	if (!valid_variable_name(name))
  	{
  		psql_error("invalid variable name: \"%s\"\n", name);
  		return false;
  	}
=20=20
- 	if (!value)
- 		return DeleteVariable(space, name);
-=20
  	for (previous =3D space, current =3D space->next;
  		 current;
  		 previous =3D current, current =3D current->next)
--- 236,248 ----
=20=20
  	if (!valid_variable_name(name))
  	{
+ 		/* Deletion of non-existent variable is not an error */
+ 		if (!value)
+ 			return true;
  		psql_error("invalid variable name: \"%s\"\n", name);
  		return false;
  	}
=20=20
  	for (previous =3D space, current =3D space->next;
  		 current;
  		 previous =3D current, current =3D current->next)
*************** SetVariable(VariableSpace space, const c
*** 249,262 ****
  		if (strcmp(current->name, name) =3D=3D 0)
  		{
  			/*
! 			 * 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 =3D pg_strdup(value);
  			bool		confirmed;
=20=20
  			if (current->assign_hook)
  				confirmed =3D (*current->assign_hook) (new_value);
  			else
--- 250,269 ----
  		if (strcmp(current->name, name) =3D=3D 0)
  		{
  			/*
! 			 * Found entry, so update, unless assign hook returns false.
! 			 *
! 			 * We must duplicate the passed value to start with.  This
! 			 * simplifies the API for substitute hooks.  Moreover, some assign
! 			 * hooks assume that the passed value has the same lifespan as the
! 			 * variable.  Having to free the string again on failure is a
! 			 * small price to pay for keeping these APIs simple.
  			 */
! 			char	   *new_value =3D value ? pg_strdup(value) : NULL;
  			bool		confirmed;
=20=20
+ 			if (current->substitute_hook)
+ 				new_value =3D (*current->substitute_hook) (new_value);
+=20
  			if (current->assign_hook)
  				confirmed =3D (*current->assign_hook) (new_value);
  			else
*************** SetVariable(VariableSpace space, const c
*** 267,288 ****
  				if (current->value)
  					pg_free(current->value);
  				current->value =3D new_value;
  			}
! 			else
  				pg_free(new_value);		/* current->value is left unchanged */
=20=20
  			return confirmed;
  		}
  	}
=20=20
  	/* not present, make new entry */
  	current =3D pg_malloc(sizeof *current);
  	current->name =3D pg_strdup(name);
! 	current->value =3D pg_strdup(value);
  	current->assign_hook =3D NULL;
  	current->next =3D NULL;
  	previous->next =3D current;
! 	return true;
  }
=20=20
  /*
--- 274,358 ----
  				if (current->value)
  					pg_free(current->value);
  				current->value =3D new_value;
+=20
+ 				/*
+ 				 * If we deleted the value, and there are no hooks to
+ 				 * remember, we can discard the variable altogether.
+ 				 */
+ 				if (new_value =3D=3D NULL &&
+ 					current->substitute_hook =3D=3D NULL &&
+ 					current->assign_hook =3D=3D NULL)
+ 				{
+ 					previous->next =3D current->next;
+ 					free(current->name);
+ 					free(current);
+ 				}
  			}
! 			else if (new_value)
  				pg_free(new_value);		/* current->value is left unchanged */
=20=20
  			return confirmed;
  		}
  	}
=20=20
+ 	/* not present, make new entry ... unless we were asked to delete */
+ 	if (value)
+ 	{
+ 		current =3D pg_malloc(sizeof *current);
+ 		current->name =3D pg_strdup(name);
+ 		current->value =3D pg_strdup(value);
+ 		current->substitute_hook =3D NULL;
+ 		current->assign_hook =3D NULL;
+ 		current->next =3D NULL;
+ 		previous->next =3D current;
+ 	}
+ 	return true;
+ }
+=20
+ /*
+  * Attach a substitute 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.  (But the hook
+  * might immediately change the NULL to something else.)
+  *
+  * The hook is immediately called on the variable's current value.
+  */
+ void
+ SetVariableSubstituteHook(VariableSpace space, const char *name,
+ 						  VariableSubstituteHook hook)
+ {
+ 	struct _variable *current,
+ 			   *previous;
+=20
+ 	if (!space || !name)
+ 		return;
+=20
+ 	if (!valid_variable_name(name))
+ 		return;
+=20
+ 	for (previous =3D space, current =3D space->next;
+ 		 current;
+ 		 previous =3D current, current =3D current->next)
+ 	{
+ 		if (strcmp(current->name, name) =3D=3D 0)
+ 		{
+ 			/* found entry, so update */
+ 			current->substitute_hook =3D hook;
+ 			current->value =3D (*hook) (current->value);
+ 			return;
+ 		}
+ 	}
+=20
  	/* not present, make new entry */
  	current =3D pg_malloc(sizeof *current);
  	current->name =3D pg_strdup(name);
! 	current->value =3D NULL;
! 	current->substitute_hook =3D hook;
  	current->assign_hook =3D NULL;
  	current->next =3D NULL;
  	previous->next =3D current;
! 	current->value =3D (*hook) (current->value);
  }
=20=20
  /*
*************** SetVariable(VariableSpace space, const c
*** 299,305 ****
   * get called before any user-supplied value is assigned.
   */
  void
! SetVariableAssignHook(VariableSpace space, const char *name, VariableAssi=
gnHook hook)
  {
  	struct _variable *current,
  			   *previous;
--- 369,376 ----
   * get called before any user-supplied value is assigned.
   */
  void
! SetVariableAssignHook(VariableSpace space, const char *name,
! 					  VariableAssignHook hook)
  {
  	struct _variable *current,
  			   *previous;
*************** SetVariableAssignHook(VariableSpace spac
*** 327,332 ****
--- 398,404 ----
  	current =3D pg_malloc(sizeof *current);
  	current->name =3D pg_strdup(name);
  	current->value =3D NULL;
+ 	current->substitute_hook =3D NULL;
  	current->assign_hook =3D hook;
  	current->next =3D NULL;
  	previous->next =3D current;
*************** SetVariableBool(VariableSpace space, con
*** 351,392 ****
  bool
  DeleteVariable(VariableSpace space, const char *name)
  {
! 	struct _variable *current,
! 			   *previous;
!=20
! 	if (!space)
! 		return true;
!=20
! 	for (previous =3D space, current =3D space->next;
! 		 current;
! 		 previous =3D current, current =3D current->next)
! 	{
! 		if (strcmp(current->name, name) =3D=3D 0)
! 		{
! 			if (current->assign_hook)
! 			{
! 				/* 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 =3D 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 =3D current->next;
! 				free(current->name);
! 				free(current);
! 			}
! 			return true;
! 		}
! 	}
!=20
! 	return true;
  }
=20=20
  /*
--- 423,429 ----
  bool
  DeleteVariable(VariableSpace space, const char *name)
  {
! 	return SetVariable(space, name, NULL);
  }
=20=20
  /*
diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.h
index 274b4af..f382bfd 100644
*** a/src/bin/psql/variables.h
--- b/src/bin/psql/variables.h
***************
*** 18,45 ****
   * 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.
   *
!  * When a hook function is installed with SetVariableAssignHook(), it is
!  * called with the variable's current value (or with NULL, if it wasn't s=
et
   * 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);
=20=20
  /*
   * Data structure representing one variable.
   *
   * Note: if value =3D=3D NULL then the variable is logically unset, but w=
e are
!  * keeping the struct around so as not to forget about its hook function.
   */
  struct _variable
  {
  	char	   *name;
  	char	   *value;
  	VariableAssignHook assign_hook;
  	struct _variable *next;
  };
--- 18,70 ----
   * prevent invalid values from being assigned, and can update internal C
   * variables to keep them in sync with the variable's current value.
   *
!  * An assign hook function is called before any attempted assignment, wit=
h 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.
   *
!  * When an assign 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);
=20=20
  /*
+  * Variables can also be given "substitute hook" functions.  The substitu=
te
+  * hook can replace values (including NULL) with other values, allowing
+  * normalization of variable contents.  For example, for a boolean variab=
le,
+  * we wish to interpret "\unset FOO" as "\set FOO off", and we can do that
+  * by installing a substitute hook.  (We can use the same substitute hook
+  * for all bool or nearly-bool variables, which is why this responsibility
+  * isn't part of the assign hook.)
+  *
+  * The substitute hook is called before any attempted assignment, and bef=
ore
+  * the assign hook if any, passing the proposed new value of the variable=
 as a
+  * malloc'd string (or NULL, if an \unset is being attempted).  It can re=
turn
+  * the same value, or a different malloc'd string, or modify the string
+  * in-place.  It should free the passed-in value if it's not returning it.
+  * The substitute hook generally should not complain about erroneous valu=
es;
+  * that's a job for the assign hook.
+  *
+  * When a substitute hook is installed with SetVariableSubstituteHook(),
+  * it is applied to the variable's current value (typically NULL, if it w=
asn't
+  * set yet).  It's usually best to do that before installing the assign h=
ook
+  * if there is one.
+  */
+ typedef char *(*VariableSubstituteHook) (char *newval);
+=20
+ /*
   * Data structure representing one variable.
   *
   * Note: if value =3D=3D NULL then the variable is logically unset, but w=
e are
!  * keeping the struct around so as not to forget about its hook function(=
s).
   */
  struct _variable
  {
  	char	   *name;
  	char	   *value;
+ 	VariableSubstituteHook substitute_hook;
  	VariableAssignHook assign_hook;
  	struct _variable *next;
  };
*************** int GetVariableNum(VariableSpace space,
*** 65,70 ****
--- 90,96 ----
  void		PrintVariables(VariableSpace space);
=20=20
  bool		SetVariable(VariableSpace space, const char *name, const char *valu=
e);
+ void		SetVariableSubstituteHook(VariableSpace space, const char *name, Va=
riableSubstituteHook hook);
  void		SetVariableAssignHook(VariableSpace space, const char *name, Variab=
leAssignHook hook);
  bool		SetVariableBool(VariableSpace space, const char *name);
  bool		DeleteVariable(VariableSpace space, const char *name);
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected=
/psql.out
index 420825a..026a4f0 100644
*** a/src/test/regress/expected/psql.out
--- b/src/test/regress/expected/psql.out
*************** invalid variable name: "invalid/name"
*** 11,16 ****
--- 11,33 ----
  unrecognized value "foo" for "AUTOCOMMIT": boolean expected
  \set FETCH_COUNT foo
  invalid value "foo" for "FETCH_COUNT": integer expected
+ -- check handling of built-in boolean variable
+ \echo :ON_ERROR_ROLLBACK
+ off
+ \set ON_ERROR_ROLLBACK
+ \echo :ON_ERROR_ROLLBACK
+ on
+ \set ON_ERROR_ROLLBACK foo
+ unrecognized value "foo" for "ON_ERROR_ROLLBACK"
+ Available values are: on, off, interactive.
+ \echo :ON_ERROR_ROLLBACK
+ on
+ \set ON_ERROR_ROLLBACK on
+ \echo :ON_ERROR_ROLLBACK
+ on
+ \unset ON_ERROR_ROLLBACK
+ \echo :ON_ERROR_ROLLBACK
+ off
  -- \gset
  select 10 as test01, 20 as test02, 'Hello' as test03 \gset pref01_
  \echo :pref01_test01 :pref01_test02 :pref01_test03
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 79624b9..d823d11 100644
*** a/src/test/regress/sql/psql.sql
--- b/src/test/regress/sql/psql.sql
***************
*** 10,15 ****
--- 10,25 ----
  -- fail: invalid value for special variable
  \set AUTOCOMMIT foo
  \set FETCH_COUNT foo
+ -- check handling of built-in boolean variable
+ \echo :ON_ERROR_ROLLBACK
+ \set ON_ERROR_ROLLBACK
+ \echo :ON_ERROR_ROLLBACK
+ \set ON_ERROR_ROLLBACK foo
+ \echo :ON_ERROR_ROLLBACK
+ \set ON_ERROR_ROLLBACK on
+ \echo :ON_ERROR_ROLLBACK
+ \unset ON_ERROR_ROLLBACK
+ \echo :ON_ERROR_ROLLBACK
=20=20
  -- \gset
=20=20

------- =_aaaaaaaaaa0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

------- =_aaaaaaaaaa0--