Re: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
Andrew Dunstan writes: > On 12/30/2014 09:20 AM, Tom Lane wrote: >> In one light this is certainly a bug fix, but in another it's just >> definitional instability. >> >> If we'd gotten a field bug report we might well have chosen to back-patch, >> though, and perhaps your client's complaint counts as that. > I got caught by this with ON_ERROR_ROLLBACK on 9.3 just this afternoon > before remembering this thread. So there's a field report :-) > +0.75 for backpatching (It's hard to imagine someone relying on the bad > behaviour, but you never know). It seems like there's a consensus in favor of back-patching this change, so I'll go ahead and do that. 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: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
On 12/30/2014 09:20 AM, Tom Lane wrote: Bernd Helmle writes: --On 29. Dezember 2014 12:55:11 -0500 Tom Lane wrote: Given the lack of previous complaints, this probably isn't backpatching material, but it sure seems like a bit of attention to consistency would be warranted here. Now that i read it i remember a client complaining about this some time ago. I forgot about it, but i think there's value in it to backpatch. Hm. Last night I wrote the attached draft patch, which I was intending to apply to HEAD only. The argument against back-patching is basically that this might change the interpretation of scripts that had been accepted silently before. For example \set ECHO_HIDDEN NoExec will now select "noexec" mode whereas before you silently got "on" mode. In one light this is certainly a bug fix, but in another it's just definitional instability. If we'd gotten a field bug report we might well have chosen to back-patch, though, and perhaps your client's complaint counts as that. Opinions anyone? r I got caught by this with ON_ERROR_ROLLBACK on 9.3 just this afternoon before remembering this thread. So there's a field report :-) +0.75 for backpatching (It's hard to imagine someone relying on the bad behaviour, but you never know). cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
On Tue, Dec 30, 2014 at 8:54 AM, Adrian Klaver wrote: > On 12/30/2014 07:43 AM, David G Johnston wrote: > >> Tom Lane-2 wrote >> >>> Bernd Helmle < >>> >> >> mailings@ >>> >> >> > writes: >>> --On 29. Dezember 2014 12:55:11 -0500 Tom Lane < >>> >> tgl@.pa >>> >> >> > wrote: >>> Given the lack of previous complaints, this probably isn't backpatching > material, but it sure seems like a bit of attention to consistency > would be warranted here. > >>> Now that i read it i remember a client complaining about this some time ago. I forgot about it, but i think there's value in it to backpatch. >>> >>> Hm. Last night I wrote the attached draft patch, which I was intending >>> to apply to HEAD only. The argument against back-patching is basically >>> that this might change the interpretation of scripts that had been >>> accepted silently before. For example >>> \set ECHO_HIDDEN NoExec >>> will now select "noexec" mode whereas before you silently got "on" mode. >>> In one light this is certainly a bug fix, but in another it's just >>> definitional instability. >>> >>> If we'd gotten a field bug report we might well have chosen to >>> back-patch, >>> though, and perhaps your client's complaint counts as that. >>> >>> Opinions anyone? >>> >> >> -0.5 for back patching >> >> The one thing supporting this is that we'd potentially be fixing scripts >> that are broken but don't know it yet. But the downside of changing >> active >> settings for working scripts - even if they are only accidentally working >> - >> is enough to counter that for me. Being more liberal in our acceptance of >> input is more feature than bug fix even if we document that we accept more >> items. >> > > It is more about being consistent then liberal. Personally I think a > situation where for one variable 0 = off but for another 0 = on, is a bug > > I can sorta buy the consistency angle but what will seal it for me is script portability - the ability to write a script and instructions using the most current release and have it run on previous versions without having to worry about this kind of incompatibility. So, +1 for back patching from me. David J.
Re: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
Bernd Helmle writes: > --On 29. Dezember 2014 12:55:11 -0500 Tom Lane wrote: >> Given the lack of previous complaints, this probably isn't backpatching >> material, but it sure seems like a bit of attention to consistency >> would be warranted here. > Now that i read it i remember a client complaining about this some time > ago. I forgot about it, but i think there's value in it to backpatch. Hm. Last night I wrote the attached draft patch, which I was intending to apply to HEAD only. The argument against back-patching is basically that this might change the interpretation of scripts that had been accepted silently before. For example \set ECHO_HIDDEN NoExec will now select "noexec" mode whereas before you silently got "on" mode. In one light this is certainly a bug fix, but in another it's just definitional instability. If we'd gotten a field bug report we might well have chosen to back-patch, though, and perhaps your client's complaint counts as that. Opinions anyone? regards, tom lane diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index d29dfa2..bdfb67c 100644 *** a/doc/src/sgml/ref/psql-ref.sgml --- b/doc/src/sgml/ref/psql-ref.sgml *** EOF *** 173,180 Echo the actual queries generated by \d and other backslash commands. You can use this to study psql's internal operations. This is equivalent to ! setting the variable ECHO_HIDDEN from within ! psql. --- 173,179 Echo the actual queries generated by \d and other backslash commands. You can use this to study psql's internal operations. This is equivalent to ! setting the variable ECHO_HIDDEN to on. *** EOF *** 333,340 quietly. By default, it prints welcome messages and various informational output. If this option is used, none of this happens. This is useful with the -c option. ! Within psql you can also set the ! QUIET variable to achieve the same effect. --- 332,339 quietly. By default, it prints welcome messages and various informational output. If this option is used, none of this happens. This is useful with the -c option. ! This is equivalent to setting the variable QUIET ! to on. *** bar *** 2884,2891 ECHO_HIDDEN ! When this variable is set and a backslash command queries the ! database, the query is first shown. This way you can study the PostgreSQL internals and provide similar functionality in your own programs. (To select this behavior on program start-up, use the switch -E.) If you set --- 2883,2891 ECHO_HIDDEN ! When this variable is set to on and a backslash command ! queries the database, the query is first shown. ! This feature helps you to study PostgreSQL internals and provide similar functionality in your own programs. (To select this behavior on program start-up, use the switch -E.) If you set *** bar *** 3046,3061 ! When on, if a statement in a transaction block generates an error, the error is ignored and the transaction ! continues. When interactive, such errors are only ignored in interactive sessions, and not when reading script ! files. When off (the default), a statement in a transaction block that generates an error aborts the entire ! transaction. The on_error_rollback-on mode works by issuing an implicit SAVEPOINT for you, just before each command ! that is in a transaction block, and rolls back to the savepoint ! on error. --- 3046,3061 ! When set to on, if a statement in a transaction block generates an error, the error is ignored and the transaction ! continues. When set to interactive, such errors are only ignored in interactive sessions, and not when reading script ! files. When unset or set to off, a statement in a transaction block that generates an error aborts the entire ! transaction. The error rollback mode works by issuing an implicit SAVEPOINT for you, just before each command ! that is in a transaction block, and then rolling back to the ! savepoint if the command fails. *** bar *** 3065,3071 By default, command processing continues after an error. When this ! variable is set, it will instead stop immediately. In interactive mode, psql will return to the command prompt; otherwise, psql
Re: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
--On 29. Dezember 2014 12:55:11 -0500 Tom Lane wrote: Given the lack of previous complaints, this probably isn't backpatching material, but it sure seems like a bit of attention to consistency would be warranted here. Now that i read it i remember a client complaining about this some time ago. I forgot about it, but i think there's value in it to backpatch. -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
On 12/29/2014 09:55 AM, Tom Lane wrote: Adrian Klaver writes: So it seems you can turn ON_ERROR_ROLLBACK on with either 1 or 'on', but you can only turn it off with 'off'. With ON_ERROR_STOP 1/on and 0/off both seem to work. Is this expected? Given the lack of previous complaints, this probably isn't backpatching material, but it sure seems like a bit of attention to consistency would be warranted here. I would appreciate it, thanks. regards, tom lane -- Adrian Klaver adrian.kla...@aklaver.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
Adrian Klaver writes: > So it seems you can turn ON_ERROR_ROLLBACK on with either 1 or 'on', but you > can only turn it off with 'off'. > With ON_ERROR_STOP 1/on and 0/off both seem to work. > Is this expected? on_error_stop_hook() uses ParseVariableBool, while on_error_rollback_hook() uses some hard-coded logic that checks for "interactive" and "off" and otherwise assumes "on". This is inconsistent first in not accepting as many spellings as ParseVariableBool, and second in not complaining about invalid input --- ParseVariableBool does, so why not here? echo_hook, echo_hidden_hook, histcontrol_hook, and verbosity_hook are equally cavalierly written. For on_error_rollback_hook and echo_hidden_hook, where "on" and "off" are documented values, I think it would make sense to allow all spellings accepted by ParseVariableBool, say like this: if (newval == NULL) pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF; else if (pg_strcasecmp(newval, "interactive") == 0) pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE; -else if (pg_strcasecmp(newval, "off") == 0) -pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF; -else -pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON; +else if (ParseVariableBool(newval)) +pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON; +else +pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF; The other 3 functions don't need to accept on/off I think, but they should print a warning for unrecognized input like ParseVariableBool does. And while we're at it, we should consistently allow case-insensitive input; right now it looks like somebody rolled dice to decide whether to use "strcmp" or "pg_strcasecmp" in these functions. Per line, not even per function. Given the lack of previous complaints, this probably isn't backpatching material, but it sure seems like a bit of attention to consistency would be warranted here. 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
