From: Tom Lane <tgl@sss.pgh.pa.us>
To: Daniel Verite <daniel@manitou-mail.org>
Cc: Rahila Syed <rahilasyed90@gmail.com>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Andrew Dunstan <andrew@dunslane.net>
Subject: Re: Improvements in psql hooks for variables
Date: Tue, 31 Jan 2017 11:52:13 -0500
Message-ID: <30629.1485881533@sss.pgh.pa.us> (raw)
In-Reply-To: <0451dd1e-aad1-4f9c-855e-6cc6584d4b54@manitou-mail.org>
References: <0451dd1e-aad1-4f9c-855e-6cc6584d4b54@manitou-mail.org>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
"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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:improve-psql-bool-var-behavior-1.patch (text/x-diff, 16.3K) downloaddiff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.cindex 0574b5b..985cfcb 100644
*** a/src/bin/psql/startup.c
--- b/src/bin/psql/startup.c
*************** showVersion(void)
*** 777,787 ****
/*
! * Assign hooks for psql variables.
*
* This isn't an amazingly good place for them, but neither is anywhere else.
*/
static bool
autocommit_hook(const char *newval)
{
--- 777,804 ----
/*
! * Substitute hooks and assign hooks for psql variables.
*
* This isn't an amazingly good place for them, but neither is anywhere else.
*/
+ static char *+ bool_substitute_hook(char *newval)+ {+ if (newval == NULL)+ {+ /* "\unset FOO" becomes "\set FOO off" */+ newval = pg_strdup("off");+ }+ else if (newval[0] == '\0')+ {+ /* "\set FOO" becomes "\set FOO on" */+ pg_free(newval);+ newval = pg_strdup("on");+ }+ return newval;+ }+
static bool
autocommit_hook(const char *newval)
{
*************** EstablishVariableSpace(void)
*** 1002,1015 ****
--- 1019,1039 ----
{
pset.vars = CreateVariableSpace();
+ SetVariableSubstituteHook(pset.vars, "AUTOCOMMIT", bool_substitute_hook);
SetVariableAssignHook(pset.vars, "AUTOCOMMIT", autocommit_hook);
+ SetVariableSubstituteHook(pset.vars, "ON_ERROR_STOP", bool_substitute_hook);
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_substitute_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.cindex 91e4ae8..61c4ccc 100644
*** a/src/bin/psql/variables.c
--- b/src/bin/psql/variables.c
*************** CreateVariableSpace(void)
*** 52,57 ****
--- 52,58 ----
ptr = pg_malloc(sizeof *ptr);
ptr->name = NULL;
ptr->value = NULL;
+ ptr->substitute_hook = NULL;
ptr->assign_hook = NULL;
ptr->next = NULL;
*************** ParseVariableBool(const char *value, con
*** 101,111 ****
size_t len;
bool valid = true;
if (value == NULL)
! {
! *result = false; /* not set -> assume "off" */
! return valid;
! }
len = strlen(value);
--- 102,110 ----
size_t len;
bool valid = true;
+ /* Treat "unset" as an empty string, which will lead to error below */
if (value == NULL)
! value = "";
len = strlen(value);
*************** ParseVariableNum(const char *value, cons
*** 152,159 ****
char *end;
long numval;
if (value == NULL)
! return false;
errno = 0;
numval = strtol(value, &end, 0);
if (errno == 0 && *end == '\0' && end != value && numval == (int) numval)
--- 151,160 ----
char *end;
long numval;
+ /* Treat "unset" as an empty string, which will lead to error below */
if (value == NULL)
! value = "";
!
errno = 0;
numval = strtol(value, &end, 0);
if (errno == 0 && *end == '\0' && end != value && numval == (int) numval)
*************** SetVariable(VariableSpace space, const c
*** 235,247 ****
if (!valid_variable_name(name))
{
psql_error("invalid variable name: \"%s\"\n", name);
return false;
}
- if (!value)- return DeleteVariable(space, name);-
for (previous = space, current = space->next;
current;
previous = current, current = current->next)
--- 236,248 ----
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;
}
for (previous = space, current = space->next;
current;
previous = current, current = current->next)
*************** SetVariable(VariableSpace space, const c
*** 249,262 ****
if (strcmp(current->name, name) == 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 = pg_strdup(value);
bool confirmed;
if (current->assign_hook)
confirmed = (*current->assign_hook) (new_value);
else
--- 250,269 ----
if (strcmp(current->name, name) == 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 = value ? pg_strdup(value) : NULL;
bool confirmed;
+ if (current->substitute_hook)+ new_value = (*current->substitute_hook) (new_value);+
if (current->assign_hook)
confirmed = (*current->assign_hook) (new_value);
else
*************** SetVariable(VariableSpace space, const c
*** 267,288 ****
if (current->value)
pg_free(current->value);
current->value = new_value;
}
! else
pg_free(new_value); /* current->value is left unchanged */
return confirmed;
}
}
/* not present, make new entry */
current = pg_malloc(sizeof *current);
current->name = pg_strdup(name);
! current->value = pg_strdup(value);
current->assign_hook = NULL;
current->next = NULL;
previous->next = current;
! return true;
}
/*
--- 274,358 ----
if (current->value)
pg_free(current->value);
current->value = new_value;
+ + /*+ * If we deleted the value, and there are no hooks to+ * remember, we can discard the variable altogether.+ */+ if (new_value == NULL &&+ current->substitute_hook == NULL &&+ current->assign_hook == NULL)+ {+ previous->next = current->next;+ free(current->name);+ free(current);+ }
}
! else if (new_value)
pg_free(new_value); /* current->value is left unchanged */
return confirmed;
}
}
+ /* not present, make new entry ... unless we were asked to delete */+ if (value)+ {+ current = pg_malloc(sizeof *current);+ current->name = pg_strdup(name);+ current->value = pg_strdup(value);+ current->substitute_hook = NULL;+ current->assign_hook = NULL;+ current->next = NULL;+ previous->next = current;+ }+ return true;+ }+ + /*+ * 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;+ + if (!space || !name)+ return;+ + if (!valid_variable_name(name))+ return;+ + for (previous = space, current = space->next;+ current;+ previous = current, current = current->next)+ {+ if (strcmp(current->name, name) == 0)+ {+ /* found entry, so update */+ current->substitute_hook = hook;+ current->value = (*hook) (current->value);+ return;+ }+ }+
/* not present, make new entry */
current = pg_malloc(sizeof *current);
current->name = pg_strdup(name);
! current->value = NULL;
! current->substitute_hook = hook;
current->assign_hook = NULL;
current->next = NULL;
previous->next = current;
! current->value = (*hook) (current->value);
}
/*
*************** SetVariable(VariableSpace space, const c
*** 299,305 ****
* get called before any user-supplied value is assigned.
*/
void
! SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook 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 = pg_malloc(sizeof *current);
current->name = pg_strdup(name);
current->value = NULL;
+ current->substitute_hook = NULL;
current->assign_hook = hook;
current->next = NULL;
previous->next = current;
*************** SetVariableBool(VariableSpace space, con
*** 351,392 ****
bool
DeleteVariable(VariableSpace space, const char *name)
{
! struct _variable *current,
! *previous;
!
! if (!space)
! return true;
!
! for (previous = space, current = space->next;
! current;
! previous = current, current = current->next)
! {
! if (strcmp(current->name, name) == 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 = 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);
! }
! return true;
! }
! }
!
! return true;
}
/*
--- 423,429 ----
bool
DeleteVariable(VariableSpace space, const char *name)
{
! return SetVariable(space, name, NULL);
}
/*
diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.hindex 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 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.
*/
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, 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 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);
/*
+ * Variables can also be given "substitute hook" functions. The substitute+ * hook can replace values (including NULL) with other values, allowing+ * normalization of variable contents. For example, for a boolean variable,+ * 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 before+ * 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 return+ * 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 values;+ * 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 wasn't+ * set yet). It's usually best to do that before installing the assign hook+ * if there is one.+ */+ typedef char *(*VariableSubstituteHook) (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(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);
bool SetVariable(VariableSpace space, const char *name, const char *value);
+ void SetVariableSubstituteHook(VariableSpace space, const char *name, VariableSubstituteHook hook);
void SetVariableAssignHook(VariableSpace space, const char *name, VariableAssignHook 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.outindex 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.sqlindex 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
-- \gset
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: tgl@sss.pgh.pa.us, daniel@manitou-mail.org, rahilasyed90@gmail.com, sfrost@snowman.net, ashutosh.bapat@enterprisedb.com, andrew@dunslane.net
Subject: Re: Improvements in psql hooks for variables
In-Reply-To: <30629.1485881533@sss.pgh.pa.us>
* 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