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 now an accepted
>> synonym
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 paradig
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 opposi
"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 the boolean built-ins.
Than
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 pat
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 approp
"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 know if the variable is g
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 DeleteVar
"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
>> effectively replace "\s
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
"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
>> "well, it's a bug in that scrip
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
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/CAH
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
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 needs to be documented.
> Y
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.
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 only one such check.
Meh
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 valu
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" writes:
> > Here's an up
"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.
regards, tom lane
--
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 \
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.
- remov
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 va
"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)
Right.
> What we could do as
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
> VERBOS
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 curre
"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 ParseVariableBool() tell:
>unrecogni
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 that the best choice is for
"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 opposite API conventions at
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 d
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_e
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 ParseVariableBool() now fai
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 include
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
> ParseVar
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.
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 main
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
It'
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
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.
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 suppose that's meant to be
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:
>
> regression=# \t
* 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 current
> behavior:
Ah, g
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
unrecognized value "foo" for "\
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
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
>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 "noexe
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;
> >+
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_h
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
--
> 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 patch. Initial comment. I got
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 m
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 +
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 let the hooks
> return a
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
54 matches
Mail list logo