Re: [HACKERS] Improvements in psql hooks for variables

On 02/01/2017 11:26 AM, Tom Lane wrote: > "Daniel Verite" 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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" 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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" 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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" 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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" 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 >>

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

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:

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

I wrote: > Rahila Syed 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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

Rahila Syed 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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

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 wrote: > "Daniel Verite"

Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" 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.

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

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. -

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" 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)

Re: [HACKERS] Improvements in psql hooks for variables

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 >

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" 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

Re: [HACKERS] Improvements in psql hooks for variables

Tom Lane wrote: > "Daniel Verite" 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

Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" 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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

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;

Re: [HACKERS] Improvements in psql hooks for variables

On Wed, Nov 23, 2016 at 11:17 PM, Daniel Verite 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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

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 >

Re: [HACKERS] Improvements in psql hooks for variables

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.

Re: [HACKERS] Improvements in psql hooks for variables

Daniel, * Daniel Verite (dan...@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

Re: [HACKERS] Improvements in psql hooks for variables

Daniel, * Daniel Verite (dan...@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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

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.

Re: [HACKERS] Improvements in psql hooks for variables

Daniel, * Daniel Verite (dan...@manitou-mail.org) wrote: > Tom Lane wrote: > > > Stephen Frost 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

Re: [HACKERS] Improvements in psql hooks for variables

Tom Lane wrote: > Stephen Frost 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:

Re: [HACKERS] Improvements in psql hooks for variables

* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost 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

Re: [HACKERS] Improvements in psql hooks for variables

Stephen Frost 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

Re: [HACKERS] Improvements in psql hooks for variables

Daniel, * Daniel Verite (dan...@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

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

>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

Re: [HACKERS] Improvements in psql hooks for variables

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; > >+

Re: [HACKERS] Improvements in psql hooks for variables

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 =

Re: [HACKERS] Improvements in psql hooks for variables

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: [HACKERS] Improvements in psql hooks for variables

> 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 wrote: > Rahila Syed wrote: > > > > I am beginning to review this

Re: [HACKERS] Improvements in psql hooks for variables

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

Re: [HACKERS] Improvements in psql hooks for variables

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 +

Re: [HACKERS] Improvements in psql hooks for variables

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 wrote: > Hi, > > Following the discussion on forbidding an AUTOCOMMIT off->on > switch mid-transaction [1], attached is a patch that

[HACKERS] Improvements in psql hooks for variables

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,