Thread
Commits
-
Clean up psql's behavior for a few more control variables.
- fd6cd698031d 10.0 landed
-
Make psql's \set display variables in alphabetical order.
- c3e3844a92fe 10.0 landed
-
Improve psql's behavior for \set and \unset of its control variables.
- 86322dc7e013 10.0 landed
-
Make psql reject attempts to set special variables to invalid values.
- 511ae628f31b 10.0 landed
-
Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-09-16T12:35:41Z
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
-
Re: Improvements in psql hooks for variables
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2016-09-19T05:51:55Z
You may want to add this to the November commitfest https://commitfest.postgresql.org/11/. On Fri, Sep 16, 2016 at 6:05 PM, Daniel Verite <daniel@manitou-mail.org> wrote: > 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 > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
-
Re: Improvements in psql hooks for variables
Rahila Syed <rahilasyed90@gmail.com> — 2016-09-19T07:40:15Z
Hello, I am beginning to review this patch. Initial comment. I got following compilation error when I applied the patch on latest sources and run make. command.c: In function ‘exec_command’: *command.c:257:5*: error: too few arguments to function ‘ParseVariableBool’ ParseVariableBool(opt1 + sizeof(prefix) - 1, prefix) ? ^ In file included from settings.h:12:0, from command.c:50: variables.h:38:7: note: declared here bool ParseVariableBool(const char *value, const char *name, bool *valid); ^ *command.c:1551:4*: error: too few arguments to function ‘ParseVariableBool’ pset.timing = ParseVariableBool(opt, "\\timing"); ^ In file included from settings.h:12:0, from command.c:50: variables.h:38:7: note: declared here bool ParseVariableBool(const char *value, const char *name, bool *valid); ^ command.c: In function ‘do_pset’: *command.c:2663:4*: error: too few arguments to function ‘ParseVariableBool’ popt->topt.expanded = ParseVariableBool(value, param); ^ In file included from settings.h:12:0, from command.c:50: variables.h:38:7: note: declared here bool ParseVariableBool(const char *value, const char *name, bool *valid); ^ *command.c:2672:4*: error: too few arguments to function ‘ParseVariableBool’ popt->topt.numericLocale = ParseVariableBool(value, param); ^ In file included from settings.h:12:0, from command.c:50: variables.h:38:7: note: declared here bool ParseVariableBool(const char *value, const char *name, bool *valid); ^ *command.c:2727:4*: error: too few arguments to function ‘ParseVariableBool’ popt->topt.tuples_only = ParseVariableBool(value, param); ^ In file included from settings.h:12:0, from command.c:50: variables.h:38:7: note: declared here bool ParseVariableBool(const char *value, const char *name, bool *valid); ^ *command.c:2759:4*: error: too few arguments to function ‘ParseVariableBool’ if (ParseVariableBool(value, param)) ^ In file included from settings.h:12:0, from command.c:50: variables.h:38:7: note: declared here bool ParseVariableBool(const char *value, const char *name, bool *valid); ^ *command.c:2781:4:* error: too few arguments to function ‘ParseVariableBool’ popt->topt.default_footer = ParseVariableBool(value, param); ^ In file included from settings.h:12:0, from command.c:50: variables.h:38:7: note: declared here bool ParseVariableBool(const char *value, const char *name, bool *valid); ^ Thank you, Rahila Syed On Mon, Sep 19, 2016 at 11:21 AM, Ashutosh Bapat < ashutosh.bapat@enterprisedb.com> wrote: > You may want to add this to the November commitfest > https://commitfest.postgresql.org/11/. > > On Fri, Sep 16, 2016 at 6:05 PM, Daniel Verite <daniel@manitou-mail.org> > wrote: > > 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 > > > > > > -- > > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > > To make changes to your subscription: > > http://www.postgresql.org/mailpref/pgsql-hackers > > > > > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-09-19T12:50:35Z
Rahila Syed wrote: > I am beginning to review this patch. Initial comment. I got following > compilation error when I applied the patch on latest sources and run make. Sorry about that, I forgot to make clean, here's an updated patch. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Ashish Tyagi <2ashishtyagi@gmail.com> — 2016-09-20T04:47:46Z
> Sorry about that, I forgot to make clean, here's an updated patch. Ongoing CMake changes will help to avoid such things, "out of source build". On Mon, Sep 19, 2016 at 6:20 PM, Daniel Verite <daniel@manitou-mail.org> wrote: > Rahila Syed wrote: > > > > I am beginning to review this patch. Initial comment. I got following > > compilation error when I applied the patch on latest sources and run > make. > > Sorry about that, I forgot to make clean, here's an updated patch. > > Best regards, > -- > Daniel Vérité > PostgreSQL-powered mailer: http://www.manitou-mail.org > Twitter: @DanielVerite > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-09-20T17:46:40Z
Ashutosh Bapat wrote: > You may want to add this to the November commitfest > https://commitfest.postgresql.org/11/. Done. It's at https://commitfest.postgresql.org/11/799/ Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Rahila Syed <rahilasyed90@gmail.com> — 2016-10-31T22:36:09Z
Hello, I have applied this patch on latest HEAD and have done basic testing which works fine. Some comments, > if (current->assign_hook) >- (*current->assign_hook) (current->value); >- return true; >+ { >+ confirmed = (*current->assign_hook) (value); >+ } >+ if (confirmed) Spurious brackets >static bool >+generic_boolean_hook(const char *newval, const char* varname, bool *flag) >+{ Contrary to what name suggests this function does not seem to have other implementations as in a hook. Also this takes care of rejecting a syntactically wrong value only for boolean variable hooks like autocommit_hook, on_error_stop_hook. However, there are other variable hooks which call ParseVariableBool. For instance, echo_hidden_hook which is handled separately in the patch. Thus there is some duplication of code between generic_boolean_hook and echo_hidden_hook. Similarly for on_error_rollback_hook. >-static void >+static bool > fetch_count_hook(const char *newval) > { > pset.fetch_count = ParseVariableNum(newval, -1, -1, false); >+ return true; > } Shouldn't invalid numeric string assignment for numeric variables be handled too? Instead of generic_boolean_hook cant we have something like follows which like generic_boolean_hook can be called from specific variable assignment hooks, static bool ParseVariable(newval, VariableName, &pset.var) { if (VariableName == ‘AUTOCOMMIT’ || ECHO_HIDDEN || other variable with hooks which call ParseVariableBool ) <logic here same as generic_boolean_hook in patch <additional lines as there in the patch for ECHO_HIDDEN, ON_ERROR_ROLLBACK> else if (VariableName == ‘FETCH_COUNT’) ParseVariableNum(); } This will help merge the logic which is to check for valid syntax before assigning and returning false on error, in one place. >@@ -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 */ Sorry for my lack of understanding, can you explain why is above change needed? Thank you, Rahila Syed On Tue, Sep 20, 2016 at 11:16 PM, Daniel Verite <daniel@manitou-mail.org> wrote: > Ashutosh Bapat wrote: > > > You may want to add this to the November commitfest > > https://commitfest.postgresql.org/11/. > > Done. It's at https://commitfest.postgresql.org/11/799/ > > Best regards, > -- > Daniel Vérité > PostgreSQL-powered mailer: http://www.manitou-mail.org > Twitter: @DanielVerite > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-11-10T21:27:19Z
Rahila Syed wrote: > I have applied this patch on latest HEAD and have done basic testing which > works fine. Thanks for reviewing this patch! > > if (current->assign_hook) > >- (*current->assign_hook) (current->value); > >- return true; > >+ { > >+ confirmed = (*current->assign_hook) (value); > >+ } > >+ if (confirmed) > > Spurious brackets OK. > >static bool > >+generic_boolean_hook(const char *newval, const char* varname, bool *flag) > >+{ > > Contrary to what name suggests this function does not seem to have other > implementations as in a hook. > Also this takes care of rejecting a syntactically wrong value only for > boolean variable hooks like autocommit_hook, > on_error_stop_hook. However, there are other variable hooks which call > ParseVariableBool. > For instance, echo_hidden_hook which is handled separately in the patch. > Thus there is some duplication of code between generic_boolean_hook and > echo_hidden_hook. > Similarly for on_error_rollback_hook. The purpose of generic_boolean_hook() is to handle the case of a boolean variable that only accepts ON or OFF, and has its pset.varname declared as bool. I thought of this case as "generic" because that's the base case and several variables need no more than that. ECHO_HIDDEN differs from the generic boolean case because it also accepts "noexec" and pset.echo_hidden is an enum, not a boolean. When considering refactoring echo_hidden_hook() to call generic_boolean_hook() instead of ParseVariableBool() after having established that the value is not "noexec", I don't see any advantage in clarity or code size, so I'm not in favor of that change. The same applies to on_error_rollback_hook(), which has to deal with a specific enum as well. > >-static void > >+static bool > > fetch_count_hook(const char *newval) > > { > > pset.fetch_count = ParseVariableNum(newval, -1, -1, false); > >+ return true; > > } > > Shouldn't invalid numeric string assignment for numeric variables be > handled too? Agreed. Assignments like "\set FETCH_COUNT bogus" don't provoke any user feedback currently, which is not ideal. I'll add this in a v3 of the patch tomorrow. > Instead of generic_boolean_hook cant we have something like follows which > like generic_boolean_hook can be called from specific variable assignment > hooks, > > static bool > ParseVariable(newval, VariableName, &pset.var) > { > if (VariableName == ‘AUTOCOMMIT’ || ECHO_HIDDEN || other variable with > hooks which call ParseVariableBool ) > <logic here same as generic_boolean_hook in patch > <additional lines as there in the patch for ECHO_HIDDEN, > ON_ERROR_ROLLBACK> > else if (VariableName == ‘FETCH_COUNT’) > ParseVariableNum(); > } It's not possible because pset.var corresponds to different fields from struct _psqlSettings that have different types: bool, int and some enum types. Besides, I don't think it would go well with hooks. If we wanted one big function that knows all about parsing all built-in variables, we could just as well dispense with hooks, since their current purpose in psql is to achieve this parsing, but in a decentralized way. Or if we keep them, our various built-in variables would be essentially tied to the same one-big-hook-that-does-all, but isn't that an antipattern for hooks? > >@@ -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 */ > > Sorry for my lack of understanding, can you explain why is above change > needed? "hook" is changed by the patch from [pointer to function returning void], to [pointer to function returning bool]. The cast to void is not essential, it just indicates that we deliberately want to ignore the return value here. I expect some compilers might complain under a high level of warnings without this cast, although TBH if you ask me, I wouldn't know which compiler with which flags exactly. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -
Re: Improvements in psql hooks for variables
Rahila Syed <rahilasyed90@gmail.com> — 2016-11-13T20:50:09Z
>ECHO_HIDDEN differs from the generic boolean case because it also >accepts "noexec" and pset.echo_hidden is an enum, not a boolean. When >considering refactoring echo_hidden_hook() to call >generic_boolean_hook() instead of ParseVariableBool() after >having established that the value is not "noexec", I don't see >any advantage in clarity or code size, so I'm not in favor of that change I agree if generic_boolean_hook() is used in its current form instead of ParseVariableBool() inside echo_hidden_hook or on_error_rollback_hook the code will not change much. I was suggesting change on the lines of extending generic_boolean_hook to include enum(part in enum_hooks which calls ParseVariableBool) and integer parsing as well. >Besides, I don't think it would go well with hooks. If we wanted one >big function that knows all about parsing all built-in variables, we >could just as well dispense with hooks, since their current purpose in >psql is to achieve this parsing, but in a decentralized way. >Or if we keep them, our various built-in variables would be >essentially tied to the same one-big-hook-that-does-all, but isn't >that an antipattern for hooks? I was suggesting something on the lines of having common parsing logic for each implementation of hook. This is similar to what ParseVariableBool does in the existing code. ParseVariableBool is being called from different hooks to parse the boolean value of the variable. Thus representing common code in various implementations of hook. >"hook" is changed by the patch from [pointer to function returning >void], to [pointer to function returning bool]. The cast to void is >not essential, it just indicates that we deliberately want to >ignore the return value here. I expect some compilers might >complain under a high level of warnings without this cast, although >TBH if you ask me, I wouldn't know which compiler with which flags >exactly. Thank you for explanation. Thank you, Rahila Syed On Fri, Nov 11, 2016 at 2:57 AM, Daniel Verite <daniel@manitou-mail.org> wrote: > Rahila Syed wrote: > > > I have applied this patch on latest HEAD and have done basic testing > which > > works fine. > > Thanks for reviewing this patch! > > > > if (current->assign_hook) > > >- (*current->assign_hook) (current->value); > > >- return true; > > >+ { > > >+ confirmed = (*current->assign_hook) (value); > > >+ } > > >+ if (confirmed) > > > > Spurious brackets > > OK. > > > >static bool > > >+generic_boolean_hook(const char *newval, const char* varname, bool > *flag) > > >+{ > > > > Contrary to what name suggests this function does not seem to have other > > implementations as in a hook. > > Also this takes care of rejecting a syntactically wrong value only for > > boolean variable hooks like autocommit_hook, > > on_error_stop_hook. However, there are other variable hooks which call > > ParseVariableBool. > > For instance, echo_hidden_hook which is handled separately in the patch. > > Thus there is some duplication of code between generic_boolean_hook and > > echo_hidden_hook. > > Similarly for on_error_rollback_hook. > > The purpose of generic_boolean_hook() is to handle the case of a > boolean variable that only accepts ON or OFF, and has its pset.varname > declared as bool. I thought of this case as "generic" because that's > the base case and several variables need no more than that. > > ECHO_HIDDEN differs from the generic boolean case because it also > accepts "noexec" and pset.echo_hidden is an enum, not a boolean. When > considering refactoring echo_hidden_hook() to call > generic_boolean_hook() instead of ParseVariableBool() after > having established that the value is not "noexec", I don't see > any advantage in clarity or code size, so I'm not in favor of that change. > > The same applies to on_error_rollback_hook(), which has to deal > with a specific enum as well. > > > >-static void > > >+static bool > > > fetch_count_hook(const char *newval) > > > { > > > pset.fetch_count = ParseVariableNum(newval, -1, -1, false); > > >+ return true; > > > } > > > > Shouldn't invalid numeric string assignment for numeric variables be > > handled too? > > Agreed. Assignments like "\set FETCH_COUNT bogus" don't provoke any > user feedback currently, which is not ideal. I'll add this in a > v3 of the patch tomorrow. > > > Instead of generic_boolean_hook cant we have something like follows which > > like generic_boolean_hook can be called from specific variable assignment > > hooks, > > > > static bool > > ParseVariable(newval, VariableName, &pset.var) > > { > > if (VariableName == ‘AUTOCOMMIT’ || ECHO_HIDDEN || other variable > with > > hooks which call ParseVariableBool ) > > <logic here same as generic_boolean_hook in patch > > <additional lines as there in the patch for ECHO_HIDDEN, > > ON_ERROR_ROLLBACK> > > else if (VariableName == ‘FETCH_COUNT’) > > ParseVariableNum(); > > } > > It's not possible because pset.var corresponds to different fields from > struct _psqlSettings that have different types: bool, int and some > enum types. > Besides, I don't think it would go well with hooks. If we wanted one > big function that knows all about parsing all built-in variables, we > could just as well dispense with hooks, since their current purpose in > psql is to achieve this parsing, but in a decentralized way. > Or if we keep them, our various built-in variables would be > essentially tied to the same one-big-hook-that-does-all, but isn't > that an antipattern for hooks? > > > > >@@ -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 */ > > > > Sorry for my lack of understanding, can you explain why is above change > > needed? > > "hook" is changed by the patch from [pointer to function returning > void], to [pointer to function returning bool]. The cast to void is > not essential, it just indicates that we deliberately want to > ignore the return value here. I expect some compilers might > complain under a high level of warnings without this cast, although > TBH if you ask me, I wouldn't know which compiler with which flags > exactly. > > > Best regards, > -- > Daniel Vérité > PostgreSQL-powered mailer: http://www.manitou-mail.org > Twitter: @DanielVerite > -
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-11-15T12:09:45Z
Hi, I'm attaching v3 of the patch with error reporting for FETCH_COUNT as mentioned upthread, and rebased on the most recent master. > I was suggesting change on the lines of extending generic_boolean_hook to > include enum(part in enum_hooks which calls ParseVariableBool) and > integer parsing as well. Well, generic_boolean_hook() is meant to change this, for instance: static void on_error_stop_hook(const char *newval) { pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP"); } into that: static bool on_error_stop_hook(const char *newval) { return generic_boolean_hook(newval, "ON_ERROR_STOP", &pset.on_error_stop); } with the goal that the assignment does not occur if "newval" is bogus. The change is really minimal. When we're dealing with enum-or-bool variables, such as for instance ECHO_HIDDEN, the patch replaces this: static void 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; } with that: 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 { 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; } The goal being again to reject a bogus assignment, as opposed to replacing it with any hardwired value. Code-wise, we can't call generic_boolean_hook() here because we need to assign a non-boolean specific value after having parsed the ON/OFF user-supplied string. More generally, it turns out that the majority of hooks are concerned by this patch, as they parse user-supplied values, but there are 4 distinct categories of variables: 1- purely ON/OFF vars: AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP 2- ON/OFF mixed with enum values: ECHO_HIDDEN, ON_ERROR_ROLLBACK 3- purely enum values: COMP_KEYWORD_CASE, HISTCONTROL, ECHO, VERBOSITY, SHOW_CONTEXT 4- numeric values: FETCH_COUNT If you suggest that the patch should refactor the implementation of hooks for case #2, only two hooks are concerned and they consist of non-mergeable enum-specific code interleaved with generic code, so I don't foresee any gain in fusioning. I have the same opinion about merging any of #1, #2, #3, #4 together. But feel free to post code implementing your idea if you disagree, maybe I just don't figure out what you have in mind. For case #3, these hooks clearly follow a common pattern, but I also don't see any benefit in an opportunistic rewrite given the nature of the functions. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -
Re: Improvements in psql hooks for variables
Stephen Frost <sfrost@snowman.net> — 2016-11-19T23:36:03Z
Daniel, * Daniel Verite (daniel@manitou-mail.org) wrote: > I'm attaching v3 of the patch with error reporting for > FETCH_COUNT as mentioned upthread, and rebased > on the most recent master. Just fyi, there seems to be some issues with this patch because setting my PROMPT1 and PROMPT2 variables result in rather odd behavior. Here's what I use: ------------- \set PROMPT1 '\033[33;1m%M(from '`hostname`').%/.%n.%> [%`date`]\033[0m\n=%x%# ' \set PROMPT2 '-%x%# ' ------------- In reviewing this patch, I also noticed that it's set up to assume a 'true' result when a variable can't be parsed by ParseVariableBool. ----------- postgres=# \timing off Timing is off. postgres=# \timing asdsa unrecognized value "asdsa" for "\timing": boolean expected Timing is on. ----------- That certainly doesn't feel right. I'm thinking that if we're going to throw an error back to the user about a value being invalid then we shouldn't change the current value. My initial thought was that perhaps we should pass the current value to ParseVariableBool() and let it return whatever the "right" answer is, however, your use of ParseVariableBool() for enums that happen to accept on/off means that won't really work. Perhaps the right answer is to flip this around a bit and treat booleans as a special case of enums and have a generic solution for enums. Consider something like: ParseVariableEnum(valid_enums, str_value, name, curr_value); 'valid_enums' would be a simple list of what the valid values are for a given variable and their corresponding value, str_value the string the user typed, name the name of the variable, and curr_value the current value of the variable. ParseVariableEnum() could then detect if the string passed in is valid or not and report to the user if it's incorrect and leave the existing value alone. This could also generically handle the question of if the string passed in is a unique prefix of a correct value by comparing it to all of the valid values and seeing if there's a unique match or not. Thoughts? Thanks! Stephen
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-19T23:44:33Z
Stephen Frost <sfrost@snowman.net> writes: > In reviewing this patch, I also noticed that it's set up to assume a > 'true' result when a variable can't be parsed by ParseVariableBool. I suppose that's meant to be backwards-compatible with the current behavior: regression=# \timing foo unrecognized value "foo" for "\timing"; assuming "on" Timing is on. but I agree that if we're changing things in this area, that would be high on my list of things to change. I think what we want going forward is to disallow setting "special" variables to invalid values, and that should hold both for regular variables that have special behaviors, and for the special-syntax cases like \timing. regards, tom lane
-
Re: Improvements in psql hooks for variables
Stephen Frost <sfrost@snowman.net> — 2016-11-19T23:49:44Z
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > In reviewing this patch, I also noticed that it's set up to assume a > > 'true' result when a variable can't be parsed by ParseVariableBool. > > I suppose that's meant to be backwards-compatible with the current > behavior: Ah, good point, however.. > but I agree that if we're changing things in this area, that would > be high on my list of things to change. I think what we want going > forward is to disallow setting "special" variables to invalid values, > and that should hold both for regular variables that have special > behaviors, and for the special-syntax cases like \timing. I completely agree with you here. We shouldn't be assuming "invalid" means "true". Thanks! Stephen
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-11-21T13:29:57Z
Tom Lane wrote: > Stephen Frost <sfrost@snowman.net> writes: > > In reviewing this patch, I also noticed that it's set up to assume a > > 'true' result when a variable can't be parsed by ParseVariableBool. > > I suppose that's meant to be backwards-compatible with the current > behavior: > > regression=# \timing foo > unrecognized value "foo" for "\timing"; assuming "on" > Timing is on. Exactly. The scope of the patch is limited to the effect of \set assignments to built-in variables. > but I agree that if we're changing things in this area, that would > be high on my list of things to change. I think what we want going > forward is to disallow setting "special" variables to invalid values, > and that should hold both for regular variables that have special > behaviors, and for the special-syntax cases like \timing. +1 Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Stephen Frost <sfrost@snowman.net> — 2016-11-21T13:41:35Z
Daniel, * Daniel Verite (daniel@manitou-mail.org) wrote: > Tom Lane wrote: > > > Stephen Frost <sfrost@snowman.net> writes: > > > In reviewing this patch, I also noticed that it's set up to assume a > > > 'true' result when a variable can't be parsed by ParseVariableBool. > > > > I suppose that's meant to be backwards-compatible with the current > > behavior: > > > > regression=# \timing foo > > unrecognized value "foo" for "\timing"; assuming "on" > > Timing is on. > > Exactly. The scope of the patch is limited to the effect > of \set assignments to built-in variables. > > > but I agree that if we're changing things in this area, that would > > be high on my list of things to change. I think what we want going > > forward is to disallow setting "special" variables to invalid values, > > and that should hold both for regular variables that have special > > behaviors, and for the special-syntax cases like \timing. > > +1 Not sure I follow your reply here. There seems to be broad agreement to improve how we handle both \set and "special" variables and the code paths are related and this patch is touching them, so it seems like the correct next step here is to adjust the patch to update the code based on that agreement. Are you working to make those changes? I'd rather we make the changes to this code once rather than push what you have now only to turn around and change it significantly again. Thanks! Stephen
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-11-21T13:44:37Z
Stephen Frost wrote: > Just fyi, there seems to be some issues with this patch because setting > my PROMPT1 and PROMPT2 variables result in rather odd behavior. Good catch! The issue is that the patch broke the assumption of prompt hooks that their argument points to a long-lived buffer. The attached v4 fixes the bug by passing to hooks a pointer to the final strdup'ed value in VariableSpace rather than temp space, as commented in SetVariable(). Also I've changed something else in ParseVariableBool(). The code assumes "false" when value==NULL, but when value is an empty string, the result was true and considered valid, due to the following test being positive when len==0 (both with HEAD or the v3 patch from this thread): if (pg_strncasecmp(value, "true", len) == 0) return true; It happens that "" as a value yields the same result than "true" for this test so it passes, but it seems rather unintentional. The resulting problem from the POV of the user is that we can do that, for instance: test=> \set AUTOCOMMIT without error message or feedback, and that leaves us without much clue about autocommit being enabled: test=> \echo :AUTOCOMMIT test=> So I've changed ParseVariableBool() in v4 to reject empty string as an invalid boolean (but not NULL since the startup logic requires NULL meaning false in the early initialization of these variables). "make check" seems OK with that, I hope it doesn't cause any regression elsewhere. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-11-21T13:49:45Z
Stephen Frost wrote: > Are you working to make those changes? I'd rather we make the changes > to this code once rather than push what you have now only to turn around > and change it significantly again. If, as a maintainer, you prefer this together in one patch, I'm fine with that. So I'll update it shortly with changes in \timing and a few other callers of ParseVariableBool(). Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Stephen Frost <sfrost@snowman.net> — 2016-11-21T13:58:24Z
Daniel, * Daniel Verite (daniel@manitou-mail.org) wrote: > "make check" seems OK with that, I hope it doesn't cause any regression > elsewhere. You can see what the code coverage of psql is in our current regression tests by going here: http://coverage.postgresql.org/src/bin/psql/index.html It's not exactly a pretty sight and certainly not all callers of ParseVariableBool() are covered. I'd strongly suggest you either do sufficient manual testing, or add regression tests, most likely using the tap test system (you can see an example of that in src/bin/pg_dump/t and in other 't' directories). You can generate that report after you make changes yourself using 'make coverage-html'. Thanks! Stephen
-
Re: Improvements in psql hooks for variables
Stephen Frost <sfrost@snowman.net> — 2016-11-21T13:59:18Z
Daniel, * Daniel Verite (daniel@manitou-mail.org) wrote: > Stephen Frost wrote: > > > Are you working to make those changes? I'd rather we make the changes > > to this code once rather than push what you have now only to turn around > > and change it significantly again. > > If, as a maintainer, you prefer this together in one patch, I'm fine > with that. So I'll update it shortly with changes in \timing and > a few other callers of ParseVariableBool(). Did you get a chance to review and consider the other comments from my initial review about how we might use a different approach for bools, et al? Thanks! Stephen
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-11-21T16:37:36Z
Stephen Frost wrote: > Are you working to make those changes? I'd rather we make the changes > to this code once rather than push what you have now only to turn around > and change it significantly again. So I went through the psql commands which don't fail on parse errors for booleans. I'd like to attract attention on \c, because I see several options. \c [-reuse-previous=BOOL] ...other args.. Current: if we can't parse BOOL, assume it's ON, and go on with reconnecting. Option1: if we can't parse BOOL, stop here, don't reconnect, set the command result as "failed", also ignoring the other arguments. Option2: maybe we want to create a difference between interactive and non-interactive use, as there's already one in handling the failure to connect through \c. The manpage says: If the connection attempt failed (wrong user name, access denied, etc.), the previous connection will only be kept if psql is in interactive mode. When executing a non-interactive script, processing will immediately stop with an error. Proposal: if interactive, same as Option1. If not interactive, -reuse-previous=bogus has the same outcome as a failure to connect. Which I think means two subcases: if it's through \i then kill the connection but don't quit psql if it's through -f then quit psql. Option3: leave this command unchanged, avoiding trouble. \timing BOOL Current: non-parseable BOOL interpreted as TRUE. Empty BOOL toggles the state. Proposal: on non-parseable BOOL, command failure and pset.timing is left unchanged. \pset [x | expanded | vertical ] BOOL \pset numericlocale BOOL \pset [tuples_only | t] BOOL \pset footer BOOL \t BOOL (falls into pset_do("tuples_only", ...)) \pset pager BOOL Current: non-parseable non-empty BOOL interpreted as TRUE. Empty BOOL toggles the on/off state. In some cases, BOOL interpretation is attempted only after specific built-in values have been ruled out first. Proposal: non-parseable BOOL implies command failure and unchanged state. About the empty string when a BOOL is expected. Only \c -reuse-previous seems concerned: #= \c -reuse-previous= acts the same as #= \c -reuse-previous=ON Proposal: handle empty as when the value is bogus. The other commands interpret this lack of value specifically to toggle the state, so it's a non-issue for them. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-11-22T13:43:55Z
Stephen Frost wrote: > That certainly doesn't feel right. I'm thinking that if we're going to > throw an error back to the user about a value being invalid then we > shouldn't change the current value. > > My initial thought was that perhaps we should pass the current value to > ParseVariableBool() and let it return whatever the "right" answer is, > however, your use of ParseVariableBool() for enums that happen to accept > on/off means that won't really work. That's not needed once ParseVariableBool() informs the caller about the validity of the boolean expression, which is what the patch already does. For instance I just implemented it for \timing and the diff consists of just that: if (opt) - pset.timing = ParseVariableBool(opt, "\\timing", NULL); + { + bool newval = ParseVariableBool(opt, "\\timing", &success); + if (success) + pset.timing = newval; + } else pset.timing = !pset.timing; That makes \timing foobar being rejected as a bad command with a proper error message and no change of state, which is just what we want. > Perhaps the right answer is to flip this around a bit and treat booleans > as a special case of enums and have a generic solution for enums. > Consider something like: > > ParseVariableEnum(valid_enums, str_value, name, curr_value); > > 'valid_enums' would be a simple list of what the valid values are for a > given variable and their corresponding value, str_value the string the > user typed, name the name of the variable, and curr_value the current > value of the variable. Firstly I'd like to insist that such a refactoring is not necessary for this patch and I feel like it would be out of place in it. That being said, if we wanted this, I think it would be successful only if we'd first change our internal variables pset.* from a struct of different types to a list of variables from some kind of common abstract type and an abstraction layer to access them. That would be an order of magnitude more sophisticated than what we have. Otherwise as I tried to explain in [1], I don't see how we could write a ParseVariableEnum() that would return different types and take variable inputs. Or if we say that ParseVariableEnum should not return the value but affect the variable directly, that would require refactoring all call sites, and what's the benefit that would justify such large changes? Plus we have two different non-mergeable concepts of variables that need this parser: psql variables from VariableSpace stored as strings, and C variables directly instantiated as native types. Also, the argument that bools are just another type of enums is legitimate in theory, but as in psql we accept any left-anchored match of true/false/on/off/0/1, it means that the enumeration of values is in fact: 0 1 t tr tru true f fa fal fals false on of off I don't see that it would help if the code treated the above like just a vanilla list of values, comparable to the other qualifiers like "auto", "expanded", "vertical", an so on, notwithstanding the fact that they don't share the same types. I think that the current code with ParseVariableBool() that only handles booleans is better in terms of separation of concerns and readability. [1] https://www.postgresql.org/message-id/fc879967-da93-43b6-aa5a-92f2d825e786@mm Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-11-23T12:17:31Z
I wrote: > So I went through the psql commands which don't fail on parse errors > for booleans > [...] Here's a v5 patch implementing the suggestions mentioned upthread: all meta-commands calling ParseVariableBool() now fail when the boolean argument can't be parsed successfully. Also includes a minor change to SetVariableAssignHook() that now returns the result of the hook it calls after installing it. It doesn't make any difference in psql behavior since callers of SetVariableAssignHook() ignore its return value, but it's more consistent with SetVariable(). Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-12-02T12:10:06Z
On Wed, Nov 23, 2016 at 11:17 PM, Daniel Verite <daniel@manitou-mail.org> wrote: > I wrote: > > > So I went through the psql commands which don't fail on parse errors > > for booleans > > [...] > > Here's a v5 patch implementing the suggestions mentioned upthread: > all meta-commands calling ParseVariableBool() now fail > when the boolean argument can't be parsed successfully. > > Also includes a minor change to SetVariableAssignHook() that now > returns the result of the hook it calls after installing it. > It doesn't make any difference in psql behavior since callers > of SetVariableAssignHook() ignore its return value, but it's > more consistent with SetVariable(). Moved to next CF with "needs review" status. Regards, Hari Babu Fujitsu Australia
-
Re: Improvements in psql hooks for variables
Rahila Syed <rahilasyed90@gmail.com> — 2016-12-02T14:54:28Z
I applied and tested the patch on latest master branch. Kindly consider following comments, ParseVariableBool(const char *value, const char *name) +ParseVariableBool(const char *value, const char *name, bool *valid) { size_t len; + if (valid) + *valid = true; psql_error("unrecognized value \"%s\" for \"%s\": boolean expected\n", + value, name); + if (valid) + *valid = false; Why do we need this? IMO, valid should be always set to true if the value is parsed to be correct. There should not be an option to the caller to not follow the behaviour of setting valid to either true/false. As it is in the current patch, all callers of ParseVariableBool follow the behaviour of setting valid with either true/false. In following examples, incorrect error message is begin displayed. “ON_ERROR_ROLLBACK” is an enum and also accepts value 'interactive' . The error message says boolean expected. postgres=# \set ON_ERROR_ROLLBACK eretere unrecognized value "eretere" for "ON_ERROR_ROLLBACK": boolean expected \set: error while setting variable Similarly for ECHO_HIDDEN which is also an enum and accepts value 'no_exec' postgres=# \set ECHO_HIDDEN NULL unrecognized value "NULL" for "ECHO_HIDDEN": boolean expected \set: error while setting variable + bool newval = ParseVariableBool(opt, "\\timing", &success); + if (success) + pset.timing = newval; + } else pset.timing = !pset.timing; if (!pset.quiet) @@ -2660,7 +2669,14 @@ 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 valid; + bool newval = ParseVariableBool(value, param, &valid); + if (valid) Should same variable names (success / valid) be used for consistency? Thank you, Rahila Syed On Wed, Nov 23, 2016 at 5:47 PM, Daniel Verite <daniel@manitou-mail.org> wrote: > I wrote: > > > So I went through the psql commands which don't fail on parse errors > > for booleans > > [...] > > Here's a v5 patch implementing the suggestions mentioned upthread: > all meta-commands calling ParseVariableBool() now fail > when the boolean argument can't be parsed successfully. > > Also includes a minor change to SetVariableAssignHook() that now > returns the result of the hook it calls after installing it. > It doesn't make any difference in psql behavior since callers > of SetVariableAssignHook() ignore its return value, but it's > more consistent with SetVariable(). > > Best regards, > -- > Daniel Vérité > PostgreSQL-powered mailer: http://www.manitou-mail.org > Twitter: @DanielVerite > -
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2016-12-23T21:54:14Z
Rahila Syed wrote: > Kindly consider following comments, Sorry for taking so long to post an update. > There should not be an option to the caller to not follow the behaviour of > setting valid to either true/false. OK, fixed. > In following examples, incorrect error message is begin displayed. > “ON_ERROR_ROLLBACK” is an enum and also > accepts value 'interactive' . The error message says boolean expected. Indeed. Fixed for all callers of ParseVariableBool() than can accept non-boolean arguments too. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-12T17:59:30Z
"Daniel Verite" <daniel@manitou-mail.org> writes: > [ psql-var-hooks-v6.patch ] I took a quick look through this. It seems to be going in generally the right direction, but here's a couple of thoughts: * It seems like you're making life hard for yourself, and confusing for readers, by having opposite API conventions at different levels. You've got ParseVariableBool returning the parsed value as function result with validity flag going into an output parameter, but the boolean variable hooks do it the other way. I'm inclined to think that the best choice is for the function result to be the ok/not ok flag, and pass the variable-to-be-modified as an output parameter. That fits better with the notion that the variable is not to be modified on failure. You're having to change every caller of ParseVariableBool anyway, so changing them a bit more doesn't seem like a problem. I think actually you don't need generic_boolean_hook() at all if you do that; it appears to do nothing except fix this impedance mismatch. Also, why aren't you using ParseVariableBool's existing ability to report errors? It seems like this: else if (value) ! { ! bool valid; ! bool newval = ParseVariableBool(value, NULL, &valid); ! if (valid) ! popt->topt.expanded = newval; ! else ! { ! psql_error("unrecognized value \"%s\" for \"%s\"\n", ! value, param); ! return false; ! } ! } is really the hard way and you could have just done - popt->topt.expanded = ParseVariableBool(value, param); + success = ParseVariableBool(value, param, &popt->topt.expanded); I'd do it the same way for ParseCheckVariableNum. Also, is there a reason why that's adding new code rather than changing ParseVariableNum? I think if we're going to have a policy that bool variables must be valid bools, there's no reason not to insist similarly for integers. * More attention is needed to comments. Most glaringly, you've changed the API for VariableAssignHook without any attention to the API spec above that typedef. (That comment block is a bit confused anyway, since half of it is an overall explanation of what the module does and the other half is an API spec for the hooks. I think I'd move the overall explanation into the file header comment.) Also, I don't like this way of explaining an output parameter: + * "valid" points to a bool reporting whether the value was valid. because it's not really clear that the function is setting that value rather than expecting it to be set to something by the caller. Assuming you take my advice in the previous point, you could document ParseVariableBool and ParseVariableNum along the lines of * Returns true if string contents represent a valid value, false if not. * If the string is valid, the value is stored into *value, else *value * remains unchanged. regards, tom lane -
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-01-16T16:27:19Z
Tom Lane wrote: > "Daniel Verite" <daniel@manitou-mail.org> writes: > > [ psql-var-hooks-v6.patch ] > > I took a quick look through this. It seems to be going in generally > the right direction, but here's a couple of thoughts: Thanks for looking into this! > I'm inclined to think that the best choice is for the function result > to be the ok/not ok flag, and pass the variable-to-be-modified as an > output parameter. That fits better with the notion that the variable > is not to be modified on failure. Agreed, if never clobbering the variable, having the valid/invalid state returned by ParseVariableBool() allows for simpler code. I'm changing it that way. > Also, why aren't you using ParseVariableBool's existing ability to report > errors? Its' because there are two cases: - either only a boolean can be given to the command or variable, in which we let ParseVariableBool() tell: unrecognized value "bogus" for "command": boolean expected - or other parameters besides boolean are acceptable, in which case we can't say "boolean expected", because in fact a boolean is no more or less expected than the other valid values. We could shave code by reducing ParseVariableBool()'s error message to: unrecognized value "bogus" for "name" clearing the distinction between [only booleans are expected] and [booleans or enum are expected]. Then almost all callers that have their own message could rely on ParseVariableBool() instead, as they did previously. Do we care about the "boolean expected" part of the error message? > else if (value) > ! { > ! bool valid; > ! bool newval = ParseVariableBool(value, NULL, &valid); > ! if (valid) > ! popt->topt.expanded = newval; > ! else > ! { > ! psql_error("unrecognized value \"%s\" for \"%s\"\n", > ! value, param); > ! return false; > ! } > ! } > is really the hard way and you could have just done > > - popt->topt.expanded = ParseVariableBool(value, param); > + success = ParseVariableBool(value, param, > &popt->topt.expanded); I get the idea, except that in this example, the compiler is unhappy because popt->topt.expanded is not a bool, and an explicit cast here would be kludgy. For the error printing part, it would go away with the above suggestion Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-16T16:42:08Z
"Daniel Verite" <daniel@manitou-mail.org> writes: > Tom Lane wrote: >> Also, why aren't you using ParseVariableBool's existing ability to report >> errors? > Its' because there are two cases: > - either only a boolean can be given to the command or variable, > in which we let ParseVariableBool() tell: > unrecognized value "bogus" for "command": boolean expected > - or other parameters besides boolean are acceptable, in which case we > can't say "boolean expected", because in fact a boolean is no more or > less expected than the other valid values. Ah. Maybe it's time for a ParseVariableEnum, or some other way of dealing with those cases in a more unified fashion. > Do we care about the "boolean expected" part of the error message? I'm not especially in love with that particular wording, but I'm doubtful that we should give up all indication of what valid values are, especially in the cases where there are more than just bool-equivalent values. I think the right thing to do here is to fix it so that the input routine has full information about all the valid values. On the backend side, we've gone to considerable lengths to make sure that error messages are helpful for such cases, eg: regression=# set backslash_quote to foo; ERROR: invalid value for parameter "backslash_quote": "foo" HINT: Available values: safe_encoding, on, off. and I think it may be worth working equally hard here. > I get the idea, except that in this example, the compiler is > unhappy because popt->topt.expanded is not a bool, and an > explicit cast here would be kludgy. Yeah, you'll need an intermediate variable if you're trying to use ParseVariableBool for such a case. regards, tom lane
-
Re: Improvements in psql hooks for variables
Ashutosh Sharma <ashu.coek88@gmail.com> — 2017-01-20T13:00:21Z
Hi, I had a quick look into this patch and it seems to me like it takes care of all the built-in variables except ENCODING type. I think we need to apply such rule for ENCODING variable too. postgres=# \echo :ENCODING UTF8 postgres=# \set ENCODING xyz postgres=# \echo :ENCODING xyz I think currently we are not even showing what are the different valid encoding names to the end users like we show it for other built-in variables VERBOSITY, ECHO etc. I mean if i run '\set VERBOSITY' followed by tab command it does show me the valid values for VERBOSITY but not for ENCODING. postgres=# \set VERBOSITY default terse verbose Moreover, I feel we are just passing the error message to end users for any bogus assignments but not the hint message showing the correct set of values that are accepted. postgres=# \set ECHO on unrecognized value "on" for "ECHO" \set: error while setting variable Above error message should also have some expected values with it. Please note that I haven't gone through the entire mail chain so not sure if above thoughts have already been raised by others. Sorry about that. With Regards, Ashutosh Sharma EnterpriseDB: http://www.enterprisedb.com On Mon, Jan 16, 2017 at 10:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Daniel Verite" <daniel@manitou-mail.org> writes: >> Tom Lane wrote: >>> Also, why aren't you using ParseVariableBool's existing ability to report >>> errors? > >> Its' because there are two cases: >> - either only a boolean can be given to the command or variable, >> in which we let ParseVariableBool() tell: >> unrecognized value "bogus" for "command": boolean expected >> - or other parameters besides boolean are acceptable, in which case we >> can't say "boolean expected", because in fact a boolean is no more or >> less expected than the other valid values. > > Ah. Maybe it's time for a ParseVariableEnum, or some other way of > dealing with those cases in a more unified fashion. > >> Do we care about the "boolean expected" part of the error message? > > I'm not especially in love with that particular wording, but I'm doubtful > that we should give up all indication of what valid values are, especially > in the cases where there are more than just bool-equivalent values. > I think the right thing to do here is to fix it so that the input routine > has full information about all the valid values. On the backend side, > we've gone to considerable lengths to make sure that error messages are > helpful for such cases, eg: > > regression=# set backslash_quote to foo; > ERROR: invalid value for parameter "backslash_quote": "foo" > HINT: Available values: safe_encoding, on, off. > > and I think it may be worth working equally hard here. > >> I get the idea, except that in this example, the compiler is >> unhappy because popt->topt.expanded is not a bool, and an >> explicit cast here would be kludgy. > > Yeah, you'll need an intermediate variable if you're trying to use > ParseVariableBool for such a case. > > 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-hackers
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-01-20T13:44:12Z
Ashutosh Sharma wrote: > postgres=# \echo :ENCODING > UTF8 > postgres=# \set ENCODING xyz > postgres=# \echo :ENCODING > xyz > > I think currently we are not even showing what are the different valid > encoding names to the end users like we show it for other built-in > variables > VERBOSITY, ECHO etc. I mean if i run '\set VERBOSITY' followed by tab > command it does show me the valid values for VERBOSITY but not for > ENCODING. Setting ENCODING has no effect, like DBNAME, USER, HOST and PORT. In a way, it's a read-only variable that's here to inform the user, not as a means to change the encoding (\encoding does that and has proper support for tab completion) What we could do as of this patch is emit an error when we try to change ENCODING, with a hook returning false and a proper error message hinting to \encoding. I'm working on adding such messages to other variables. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-20T14:02:10Z
"Daniel Verite" <daniel@manitou-mail.org> writes: > Setting ENCODING has no effect, like DBNAME, USER, HOST and PORT. > In a way, it's a read-only variable that's here to inform the user, > not as a means to change the encoding (\encoding does that and > has proper support for tab completion) Right. > What we could do as of this patch is emit an error when we try > to change ENCODING, with a hook returning false and > a proper error message hinting to \encoding. I think that the current behavior is intentional: it avoids making those variables reserved. That is, if you're unaware that psql sets them and try to use them for your own purposes, it will work. However, it only really works if psql never overwrites the values after startup, whereas I believe all of these are overwritten by a \c command. So maybe it's more user-friendly to make these variables fully reserved, even at the risk of breaking existing scripts. But I don't think it's exactly an open-and-shut question. regards, tom lane
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-01-24T16:54:09Z
Tom Lane wrote: > However, it only really works if psql never overwrites the values > after startup, whereas I believe all of these are overwritten by > a \c command. Yes, there are reset to reflect the properties of the new connection. > So maybe it's more user-friendly to make these variables fully > reserved, even at the risk of breaking existing scripts. But > I don't think it's exactly an open-and-shut question. You mean if we make that fail: \set ENCODING UTF8 it's going to make that fail too: SELECT something AS "ENCODING"[,...] \gset and I agree it's not obvious that this trade-off has to be made. Personally I'm fine with the status quo and will not add that hook into the patch unless pressed to. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-01-24T16:59:10Z
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
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-01-24T17:03:33Z
I just wrote: > - 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 There's no such thing as \pager, I meant to write: \pset x, \pset pager, and \set of ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE, HISTCONTROL, VERBOSITY, SHOW_CONTEXT Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-24T20:03:04Z
"Daniel Verite" <daniel@manitou-mail.org> writes: > Here's an update with these changes: I scanned this patch very quickly and think it addresses my previous stylistic objections. Rahila is the reviewer of record though, so I'll wait for his comments before going further. regards, tom lane
-
Re: Improvements in psql hooks for variables
Rahila Syed <rahilasyed90@gmail.com> — 2017-01-25T12:18:54Z
Hello, The patch works fine on applying on latest master branch and testing it for various variables as listed in PsqlSettings struct. I will post further comments on patch soon. Thank you, Rahila Syed On Wed, Jan 25, 2017 at 1:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Daniel Verite" <daniel@manitou-mail.org> writes: > > Here's an update with these changes: > > I scanned this patch very quickly and think it addresses my previous > stylistic objections. Rahila is the reviewer of record though, so > I'll wait for his comments before going further. > > regards, tom lane >
-
Re: Improvements in psql hooks for variables
Rahila Syed <rahilasyed90@gmail.com> — 2017-01-30T05:38:59Z
Hello, Please consider following comments on the patch. In function ParseVariableNum, > if (!val || !val[0]) > return false; Check for 'val == NULL' as in above condition is done even in callers of ParseVariableNum(). There should be only one such check. >+ psql_error("Invalid value \"%s\" for \"%s\"\nAn integer is expected\n", Cant we have this error in ParseVariableNum() similar to ParseVariableBool() ? >+ /* >+ * 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); >+ } The above change in autocommit behaviour needs to be documented. Thank you, Rahila Syed On Wed, Jan 25, 2017 at 5:48 PM, Rahila Syed <rahilasyed90@gmail.com> wrote: > Hello, > > The patch works fine on applying on latest master branch and testing it > for various variables as listed in PsqlSettings struct. > I will post further comments on patch soon. > > Thank you, > Rahila Syed > > On Wed, Jan 25, 2017 at 1:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> "Daniel Verite" <daniel@manitou-mail.org> writes: >> > Here's an update with these changes: >> >> I scanned this patch very quickly and think it addresses my previous >> stylistic objections. Rahila is the reviewer of record though, so >> I'll wait for his comments before going further. >> >> regards, tom lane >> > > -
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-30T17:47:44Z
Rahila Syed <rahilasyed90@gmail.com> writes: > Please consider following comments on the patch. > In function ParseVariableNum, >> if (!val || !val[0]) >> return false; > Check for 'val == NULL' as in above condition is done even in callers of > ParseVariableNum(). > There should be only one such check. Meh ... I don't think it's unreasonable for ParseVariableNum to defend itself against that. The callers might or might not be applying a check --- for them, it would be a matter of do they need to detect presence or absence of an option, but not about whether the option value is valid. >> + psql_error("Invalid value \"%s\" for \"%s\"\nAn integer is >> expected\n", > Cant we have this error in ParseVariableNum() similar to > ParseVariableBool() ? Agreed, error messages should be stylistically similar. I'm not sure they can be exactly alike though. Right now the patch has ParseVariableBool saying + psql_error("unrecognized value \"%s\" for \"%s\": boolean expected\n", + value, name); while callers that don't use that error use PsqlVarEnumError which has + psql_error("Unrecognized value \"%s\" for \"%s\"\nAvailable values: %s\n", + value, name, suggestions); and then ParseVariableNum is as above. So first off we've got a capitalization inconsistency. Project style for backend messages is no initial cap; psql seems not to be on board with that entirely, but I'm definitely inclined to go with it here. As for "invalid" vs. "unrecognized", I'm not sure "unrecognized" really fits the bill for "this isn't an integer". So I'm inclined to leave that alone. I suggest we go with these: "invalid value \"%s\" for \"%s\": integer expected\n" "unrecognized value \"%s\" for \"%s\": boolean expected\n" "unrecognized value \"%s\" for \"%s\"\nAvailable values are: %s\n." where the last case is following the message style for hints. >> + * Switching AUTOCOMMIT from OFF to ON is not allowed when inside a >> + * transaction, because it cannot be effective until the current >> + * transaction is ended. > The above change in autocommit behaviour needs to be documented. Yeah, definitely. I'll go ahead and push the patch with these fixes. Thanks for reviewing! regards, tom lane -
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-30T18:17:42Z
BTW, while testing this patch I noticed that the error reports are a tad redundant: regression=# \set AUTOCOMMIT foo unrecognized value "foo" for "AUTOCOMMIT": boolean expected \set: error while setting variable regression=# The "error while setting variable" message seems entirely content-free. I think we should drop that and instead establish a rule that SetVariable should print a message for itself about any failure. (There are a lot of call sites that don't check for or print a message, but that's only because they aren't expecting failure. If one were to happen, printing a message doesn't seem unreasonable.) That would in turn propagate into an API requirement that var hooks that return FALSE are responsible for printing a message about the reason, which is why it would be appropriate to make that change as part of this patch. Barring objections PDQ, I'll make this change. regards, tom lane
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-30T20:41:33Z
I wrote: > Rahila Syed <rahilasyed90@gmail.com> writes: >>> + * Switching AUTOCOMMIT from OFF to ON is not allowed when inside a >>> + * transaction, because it cannot be effective until the current >>> + * transaction is ended. >> The above change in autocommit behaviour needs to be documented. > Yeah, definitely. Actually ... while trying to write some documentation for that, I found myself wondering why we need such a prohibition at all. If you are inside a transaction, then autocommit has no effect until after you get out of the transaction, and the documentation about it seems clear enough on the point. Also, if you want to argue that allowing it to change intra- transaction is too confusing, why would we only forbid this direction of change and not both directions? I'm afraid we might be breaking some peoples' scripts to no particularly good end, so I'm going to leave this out of the committed patch. If you think this really is a valid change to make, we can commit it separately, but let's discuss it on its own merits. regards, tom lane
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-31T00:13:40Z
So I pushed this, and the buildfarm members that are testing RedisFDW immediately fell over: *** /home/andrew/bf/root/HEAD/redis_fdw.build/test/expected/redis_fdw.out 2017-01-30 18:20:27.440677318 -0500 --- /home/andrew/bf/root/HEAD/redis_fdw.build/test/results/redis_fdw.out 2017-01-30 18:32:33.404677320 -0500 *************** *** 26,31 **** --- 26,32 ---- options (database '15', tabletype 'zset'); -- make sure they are all empty - if any are not stop the script right now \set ON_ERROR_STOP + unrecognized value "" for "ON_ERROR_STOP": boolean expected do $$ declare rows bigint; ====================================================================== Evidently, this test script is relying on the preceding behavior that setting a bool variable to an empty string was equivalent to setting it to "true". If it's just that script I would be okay with saying "well, it's a bug in that script" ... but I'm a bit worried that this may be the tip of the iceberg, ie maybe a lot of people have done things like this. Should we reconsider the decision to reject empty strings in ParseVariableBool? regards, tom lane -
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-01-31T00:19:18Z
Tom Lane wrote: > Also, if you want to argue that allowing it to change intra- > transaction is too confusing, why would we only forbid this direction > of change and not both directions? The thread "Surprising behaviour of \set AUTOCOMMIT ON" at: https://www.postgresql.org/message-id/CAH2L28sTP-9dio3X1AaZRyWb0-ANAx6BDBi37TGmvw1hBiu0oA%40mail.gmail.com seemed to converge towards the conclusion implemented in the autocommit_hook proposed in the patch. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-01-31T00:44:38Z
Tom Lane wrote: > Evidently, this test script is relying on the preceding behavior that > setting a bool variable to an empty string was equivalent to setting > it to "true". If it's just that script I would be okay with saying > "well, it's a bug in that script" ... but I'm a bit worried that this > may be the tip of the iceberg, ie maybe a lot of people have done > things like this. Should we reconsider the decision to reject empty > strings in ParseVariableBool? Sigh. It was considered upthread, I'm requoting the relevant bit: <quote> if (pg_strncasecmp(value, "true", len) == 0) return true; It happens that "" as a value yields the same result than "true" for this test so it passes, but it seems rather unintentional. The resulting problem from the POV of the user is that we can do that, for instance: test=> \set AUTOCOMMIT without error message or feedback, and that leaves us without much clue about autocommit being enabled: test=> \echo :AUTOCOMMIT test=> So I've changed ParseVariableBool() in v4 to reject empty string as an invalid boolean (but not NULL since the startup logic requires NULL meaning false in the early initialization of these variables). "make check" seems OK with that, I hope it doesn't cause any regression elsewhere. </quote> So it does cause regressions. I don't know if we should reaccept empty strings immediately to fix that, but in the long run, I think that the above situation with the empty :AUTOCOMMIT is not really sustainable: when we extend what we do with variables (/if /endif and so on), it will become even more of a problem. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-31T01:16:29Z
"Daniel Verite" <daniel@manitou-mail.org> writes: > Tom Lane wrote: >> Evidently, this test script is relying on the preceding behavior that >> setting a bool variable to an empty string was equivalent to setting >> it to "true". If it's just that script I would be okay with saying >> "well, it's a bug in that script" ... but I'm a bit worried that this >> may be the tip of the iceberg, ie maybe a lot of people have done >> things like this. Should we reconsider the decision to reject empty >> strings in ParseVariableBool? > ... > So it does cause regressions. I don't know if we should reaccept > empty strings immediately to fix that, but in the long run, I think > that the above situation with the empty :AUTOCOMMIT is not really > sustainable: when we extend what we do with variables > (/if /endif and so on), it will become even more of a problem. Yeah, in a green field we'd certainly not allow this, but the problem I'm having is that in all previous versions you could do, eg, \set ON_ERROR_STOP ... stuff expecting ON_ERROR_STOP to be on \unset ON_ERROR_STOP ... stuff expecting ON_ERROR_STOP to be off and it works, and looks perfectly natural, and people may well be relying on that. Especially since the docs aren't very clear that this shouldn't work --- psql-ref.sgml repeatedly uses phrases like "FOO is set" to indicate that the boolean variable FOO is considered to be "on". Moreover, the committed patch is inconsistent in that it forbids only one of the above. Why is it okay to treat unset as "off", but not okay to treat the default empty-string value as "on"? Maybe it's worth breaking backwards compatibility on this point, but I'm feeling unconvinced. This seems rather different from rejecting clearly-wrongly-spelled values. 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. regards, tom lane
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-01-31T11:57:45Z
Tom Lane wrote: > Moreover, the committed patch is inconsistent in that it forbids > only one of the above. Why is it okay to treat unset as "off", > but not okay to treat the default empty-string value as "on"? Treating unset (NULL in the value) as "off" comes from the fact that except AUTOCOMMIT, our built-in variables are not initialized with a default value. For instance we call this in EstablishVariableSpace(); SetVariableAssignHook(pset.vars, "ON_ERROR_STOP", on_error_stop_hook); then on_error_stop_hook is called with NULL as the value then ParseVariableBool(NULL, "ON_ERROR_STOP", &pset.on_error_stop) is what initializes pset.on_error_stop to false. The same happens if/when the variable is unset. Again the hook is called with NULL, and it sets back the pset.* variable in a hardcoded default state, which is false for all booleans. Incidentally I want to suggest to change that, so that all variables should be initialized with a non-NULL value right from the start, and the value would possibly come to NULL only if it's unset. This would allow the hook to distinguish between initialization and unsetting, which in turn will allow it to deny the \unset in the cases when it doesn't make any sense conceptually (like AUTOCOMMIT). Forcing values for our built-in variables would also avoid the following: =# \echo :ON_ERROR_STOP :ON_ERROR_STOP Even if the variable ON_ERROR_STOP does exist in the VariableSpace and has a hook and there's an initialized corresponding pset.on_error_stop, from the user's viewpoint, it's as if the variable doesn't exist until they intialize it explicitly. I suggest that it doesn't make much sense and it would be better to have that instead right from the start: =# \echo :ON_ERROR_STOP off > 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. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-31T16:52:13Z
"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
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-01-31T17:12:03Z
I wrote: > This would allow the hook to distinguish between initialization and > unsetting, which in turn will allow it to deny the \unset in the > cases when it doesn't make any sense conceptually (like AUTOCOMMIT). I notice that in the commited patch, you added the ability for DeleteVariable() to reject the deletion if the hook disagrees. + { + /* Allow deletion only if hook is okay with NULL value */ + if (!(*current->assign_hook) (NULL)) + return false; /* message printed by hook */ But this can't happen in practice because as mentioned just upthread the hook called with NULL doesn't know if the variable is getting unset or initialized, so rejecting on NULL is not an option. Attached is a proposed patch to add initial values to SetVariableAssignHook() to solve this problem, and an example for \unset AUTOCOMMIT being denied by the hook. As a side effect, we see all buit-in variables when issuing \set rather than just a few. Does it make sense? Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-31T18:07:22Z
"Daniel Verite" <daniel@manitou-mail.org> writes: > I notice that in the commited patch, you added the ability > for DeleteVariable() to reject the deletion if the hook > disagrees. Right. > But this can't happen in practice because as mentioned just upthread > the hook called with NULL doesn't know if the variable is getting unset > or initialized, so rejecting on NULL is not an option. It would have required the caller to set a value before installing the hook, which would require some reshuffling of responsibility. In the draft patch I sent a little bit ago, this is handled by installing a "substitution hook" first, and that hook transmogrifies NULL into an allowed setting. That gets to the same place in a slightly different way, but it also covers allowing "\unset FOO", which inserting initial values wouldn't. > Attached is a proposed patch to add initial values to > SetVariableAssignHook() to solve this problem, and an example for > \unset AUTOCOMMIT being denied by the hook. I think disallowing \unset is a nonstarter on compatibility grounds. We should allow \unset but treat it like setting to "off" (or whatever the default value is). regards, tom lane
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-31T21:07:12Z
I wrote: > 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. Attached is a finished version that includes hooks for all the variables for which they were clearly sensible. (FETCH_COUNT doesn't seem to really need one, and I didn't do anything with HISTSIZE or IGNOREEOF either. It might be worth bringing the latter two into the hooks paradigm, but that seems like fit material for a separate patch.) I updated the documentation as well. I think this is committable if there are not objections. regards, tom lane
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-31T22:13:06Z
BTW ... while I've been fooling with this issue, I've gotten a bit annoyed at the fact that "\set" prints the variables in, essentially, creation order. That makes the list ugly and hard to find things in, and it exposes some psql implementation details to users. I propose the attached simple patch to keep the list in name order instead. (This is on top of my previous patch, but it'd be pretty trivial to modify to apply against HEAD.) regards, tom lane
-
Re: Improvements in psql hooks for variables
Daniel Verite <daniel@manitou-mail.org> — 2017-02-01T16:18:58Z
Tom Lane wrote: > I updated the documentation as well. I think this is committable if > there are not objections. That works for me. I tested and read it and did not find anything unexpected or worth objecting. "\unset var" acting as "\set var off" makes sense considering that its opposite "\set var" is now an accepted synonym of "\set var on" for the boolean built-ins. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-01T16:26:55Z
"Daniel Verite" <daniel@manitou-mail.org> writes: > That works for me. I tested and read it and did not find anything > unexpected or worth objecting. > "\unset var" acting as "\set var off" makes sense considering > that its opposite "\set var" is now an accepted > synonym of "\set var on" for the boolean built-ins. Thanks for reviewing! I've pushed this now --- Andrew, you should be able to revert the RedisFDW test script to the way it was. regards, tom lane
-
Re: Improvements in psql hooks for variables
Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-01T18:32:53Z
I wrote: > Attached is a finished version that includes hooks for all the variables > for which they were clearly sensible. (FETCH_COUNT doesn't seem to really > need one, and I didn't do anything with HISTSIZE or IGNOREEOF either. > It might be worth bringing the latter two into the hooks paradigm, but > that seems like fit material for a separate patch.) I got more excited about doing that after noticing that not only would it clean up the behavior of those particular variables, but we could get rid of some code. First, we'd not need the rather warty GetVariableNum() anymore, and second, we'd then be almost at the point where every control variable has a hook, and therefore we could drop tab-complete.c's private list of known variable names. That was only ever needed to cover the possibility of important variables not being present in the variables list. So the attached proposed patch does these things: 1. Modify FETCH_COUNT to always have a defined value, like other control variables, mainly so it will always appear in "\set" output. 2. Add hooks to force HISTSIZE to be defined and require it to have an integer value. (I don't see any point in allowing it to be set to non-integral values.) 3. Add hooks to force IGNOREEOF to be defined and require it to have an integer value. Unlike the other cases, here we're trying to be bug-compatible with a rather bogus externally-defined behavior, so I think we need to continue to allow "\set IGNOREEOF whatever". What I propose is that the substitution hook silently replace non-numeric values with "10", so that the stored value always reflects what we're really doing. 4. Add a dummy assign hook for HISTFILE, just so it's always in variables.c's list. We can't require it to be defined always, because that would break the interaction with the PSQL_HISTORY environment variable, so there isn't any change in visible behavior here. 5. Remove tab-complete.c's private list of known variable names. Given the other changes, there are no control variables it won't show anyway. This does mean that if for some reason you've unset one of the status variables (DBNAME, HOST, etc), that variable would not appear in tab completion for \set. But I think that's fine, for at least two reasons: we shouldn't be encouraging people to use those variables as regular variables, and if someone does do so anyway, why shouldn't it act just like a regular variable? 6. Remove no-longer-used-anywhere GetVariableNum(). Any objections? regards, tom lane
-
Re: Improvements in psql hooks for variables
Andrew Dunstan <andrew@dunslane.net> — 2017-02-02T08:38:47Z
On 02/01/2017 11:26 AM, Tom Lane wrote: > "Daniel Verite" <daniel@manitou-mail.org> writes: >> That works for me. I tested and read it and did not find anything >> unexpected or worth objecting. >> "\unset var" acting as "\set var off" makes sense considering >> that its opposite "\set var" is now an accepted >> synonym of "\set var on" for the boolean built-ins. > Thanks for reviewing! I've pushed this now --- Andrew, you should > be able to revert the RedisFDW test script to the way it was. > > Done. cheers andrew