Re: [GENERAL] ON_ERROR_ROLLBACK

While working on the thread 'Rollback on include error in psql' I ran across something I am not sure with regards to ON_ERROR_ROLLBACK:

aklaver(at)panda:~> psql -d test -U aklaver -p 5452 --single-transaction --set ON_ERROR_STOP=on --set AUTOCOMMIT=off -f test_script.sql
UPDATE 1
psql:test_script.sql:2: some_missing_file.sql: No such file or directory

aklaver-2014-12-29 08:44:32.443 PST-0LOG: statement: BEGIN
aklaver-2014-12-29 08:44:32.443 PST-0LOG: statement: UPDATE testtbl SET col = 'some other value';
aklaver-2014-12-29 08:44:32.444 PST-129436LOG: statement: COMMIT

aklaver(at)panda:~> psql -d test -U aklaver -p 5452 --single-transaction --set ON_ERROR_ROLLBACK=1 --set AUTOCOMMIT=off -f test_script.sql
UPDATE 1
psql:test_script.sql:2: some_missing_file.sql: No such file or directory
UPDATE 1

aklaver-2014-12-29 08:44:42.656 PST-0LOG: statement: BEGIN
aklaver-2014-12-29 08:44:42.657 PST-0LOG: statement: SAVEPOINT pg_psql_temporary_savepoint
aklaver-2014-12-29 08:44:42.657 PST-0LOG: statement: UPDATE testtbl SET col = 'some other value';
aklaver-2014-12-29 08:44:42.658 PST-129437LOG: statement: RELEASE pg_psql_temporary_savepoint
aklaver-2014-12-29 08:44:42.658 PST-129437LOG: statement: SAVEPOINT pg_psql_temporary_savepoint
aklaver-2014-12-29 08:44:42.658 PST-129437LOG: statement: UPDATE testtbl SET col = 'yet another value';
aklaver-2014-12-29 08:44:42.659 PST-129437LOG: statement: RELEASE pg_psql_temporary_savepoint
aklaver-2014-12-29 08:44:42.659 PST-129437LOG: statement: COMMIT

aklaver(at)panda:~> psql -d test -U aklaver -p 5452 --single-transaction --set ON_ERROR_ROLLBACK=0 --set AUTOCOMMIT=off -f test_script.sql
UPDATE 1
psql:test_script.sql:2: some_missing_file.sql: No such file or directory
UPDATE 1

aklaver-2014-12-29 08:46:23.113 PST-0LOG: statement: BEGIN
aklaver-2014-12-29 08:46:23.114 PST-0LOG: statement: SAVEPOINT pg_psql_temporary_savepoint
aklaver-2014-12-29 08:46:23.114 PST-0LOG: statement: UPDATE testtbl SET col = 'some other value';
aklaver-2014-12-29 08:46:23.115 PST-129440LOG: statement: RELEASE pg_psql_temporary_savepoint
aklaver-2014-12-29 08:46:23.115 PST-129440LOG: statement: SAVEPOINT pg_psql_temporary_savepoint
aklaver-2014-12-29 08:46:23.115 PST-129440LOG: statement: UPDATE testtbl SET col = 'yet another value';
aklaver-2014-12-29 08:46:23.116 PST-129440LOG: statement: RELEASE pg_psql_temporary_savepoint
aklaver-2014-12-29 08:46:23.116 PST-129440LOG: statement: COMMIT

aklaver(at)panda:~> psql -d test -U aklaver -p 5452 --single-transaction --set ON_ERROR_ROLLBACK=off --set AUTOCOMMIT=off -f test_script.sql
UPDATE 1
psql:test_script.sql:2: some_missing_file.sql: No such file or directory
UPDATE 1

aklaver-2014-12-29 08:46:57.344 PST-0LOG: statement: BEGIN
aklaver-2014-12-29 08:46:57.345 PST-0LOG: statement: UPDATE testtbl SET col = 'some other value';
aklaver-2014-12-29 08:46:57.346 PST-129443LOG: statement: UPDATE testtbl SET col = 'yet another value';
aklaver-2014-12-29 08:46:57.346 PST-129443LOG: statement: COMMIT

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?

--
Adrian Klaver
adrian(dot)klaver(at)aklaver(dot)com

On 12/29/2014 08:51 AM, Adrian Klaver wrote:
> While working on the thread 'Rollback on include error in psql' I ran across something I am not sure with regards to ON_ERROR_ROLLBACK:
>

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

Should have mentioned Postgres version is 9.3.5

--
Adrian Klaver
adrian(dot)klaver(at)aklaver(dot)com

Adrian Klaver <adrian(dot)klaver(at)aklaver(dot)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

On 12/29/2014 09:55 AM, Tom Lane wrote:
> Adrian Klaver <adrian(dot)klaver(at)aklaver(dot)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(dot)klaver(at)aklaver(dot)com

--On 29. Dezember 2014 12:55:11 -0500 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Bernd Helmle <mailings(at)oopsware(dot)de> writes:
> --On 29. Dezember 2014 12:55:11 -0500 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Tom Lane-2 wrote
> Bernd Helmle &lt;

> mailings@

> &gt; writes:
>> --On 29. Dezember 2014 12:55:11 -0500 Tom Lane &lt;

> tgl(at)(dot)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. That said it may be worth a documentation change and release note
that those options are not liberal currently so as to help those relying on
issues find and fix them proactively.

David J.

--
View this message in context: http://postgresql.nabble.com/ON-ERROR-ROLLBACK-tp5832298p5832448.html
Sent from the PostgreSQL - general mailing list archive at Nabble.com.

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(at)(dot)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

That said it may be worth a documentation change and release note
> that those options are not liberal currently so as to help those relying on
> issues find and fix them proactively.
>
> David J.
>
>
>
>
> --
> View this message in context: http://postgresql.nabble.com/ON-ERROR-ROLLBACK-tp5832298p5832448.html
> Sent from the PostgreSQL - general mailing list archive at Nabble.com.
>
>

--
Adrian Klaver
adrian(dot)klaver(at)aklaver(dot)com

On Tue, Dec 30, 2014 at 8:54 AM, Adrian Klaver <adrian(dot)klaver(at)aklaver(dot)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(at)(dot)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.​

On 12/30/2014 09:20 AM, Tom Lane wrote:
> Bernd Helmle <mailings(at)oopsware(dot)de> writes:
>> --On 29. Dezember 2014 12:55:11 -0500 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Andrew Dunstan <andrew(at)dunslane(dot)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