Re: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
Andrew Dunstan and...@dunslane.net 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 29. Dezember 2014 12:55:11 -0500 Tom Lane t...@sss.pgh.pa.us 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
Bernd Helmle maili...@oopsware.de writes: --On 29. Dezember 2014 12:55:11 -0500 Tom Lane t...@sss.pgh.pa.us 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 command\d/command and other backslash commands. You can use this to study applicationpsql/application's internal operations. This is equivalent to ! setting the variable varnameECHO_HIDDEN/varname from within ! applicationpsql/application. /para /listitem /varlistentry --- 173,179 Echo the actual queries generated by command\d/command and other backslash commands. You can use this to study applicationpsql/application's internal operations. This is equivalent to ! setting the variable varnameECHO_HIDDEN/varname to literalon/. /para /listitem /varlistentry *** 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 option-c/option option. ! Within applicationpsql/application you can also set the ! varnameQUIET/varname variable to achieve the same effect. /para /listitem /varlistentry --- 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 option-c/option option. ! This is equivalent to setting the variable varnameQUIET/varname ! to literalon/. /para /listitem /varlistentry *** bar *** 2884,2891 termvarnameECHO_HIDDEN/varname/term listitem para ! When this variable is set and a backslash command queries the ! database, the query is first shown. This way you can study the productnamePostgreSQL/productname internals and provide similar functionality in your own programs. (To select this behavior on program start-up, use the switch option-E/option.) If you set --- 2883,2891 termvarnameECHO_HIDDEN/varname/term listitem para ! When this variable is set to literalon/ and a backslash command ! queries the database, the query is first shown. ! This feature helps you to study productnamePostgreSQL/productname internals and provide similar functionality in your own programs. (To select this behavior on program start-up, use the switch option-E/option.) If you set *** bar *** 3046,3061 /term listitem para ! When literalon/, if a statement in a transaction block generates an error, the error is ignored and the transaction ! continues. When literalinteractive/, such errors are only ignored in interactive sessions, and not when reading script ! files. When literaloff/ (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 commandSAVEPOINT/ for you, just before each command ! that is in a transaction block, and rolls back to the savepoint ! on error. /para /listitem /varlistentry --- 3046,3061 /term listitem para ! When set to literalon/, if a statement in a transaction block generates an error, the error is ignored and the transaction ! continues. When set to literalinteractive/, such errors are only ignored in interactive sessions, and not when reading script ! files. When unset or set to literaloff/, a statement in a
Re: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
On Tue, Dec 30, 2014 at 8:54 AM, Adrian Klaver adrian.kla...@aklaver.com wrote: On 12/30/2014 07:43 AM, David G Johnston wrote: Tom Lane-2 wrote Bernd Helmle lt; mailings@ gt; writes: --On 29. Dezember 2014 12:55:11 -0500 Tom Lane lt; tgl@.pa gt; 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
On 12/30/2014 09:20 AM, Tom Lane wrote: Bernd Helmle maili...@oopsware.de writes: --On 29. Dezember 2014 12:55:11 -0500 Tom Lane t...@sss.pgh.pa.us 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
Adrian Klaver adrian.kla...@aklaver.com 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
Re: [HACKERS] [GENERAL] ON_ERROR_ROLLBACK
On 12/29/2014 09:55 AM, Tom Lane wrote: Adrian Klaver adrian.kla...@aklaver.com 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
