Re: [HACKERS] New parameter RollbackError to control rollback behavior on error

Hi all,

The behavior of rollback when an error occurs on an handle is
controlled by extending Protocol with "$PROTNUM-[0|1|2]" where:
- 0 = let the application handle rollbacks
- 1 = rollback whole transaction when an error occurs
- 2 = rollback only statement that failed
Using such an extension is somewhat awkward as a single string is used
for two settings... The proposed attached patch adds a new parameter
called RollbackError that allows to control the behavior of rollback
on error with a different parameter.

For backward-compatibility purposes, this patch does not break the old
grammar of Protocol: it just gives the priority to RollbackError if
both Protocol and RollbackError are set for a connection. Regression
tests to test RollbackError and combinations of RollbackError/Protocol
are added in the patch in the existing test error-rollback (which has
needed some refactoring, older tests are not impacted). Docs are
included as well.

I thought first about including that in my cleanup work for 9.4, but
as this actually does not break the driver it may be worth adding it
directly to master, explaining the patch attached here. Comments
welcome. Note that if there are objections I do not mind adding that
for the work that would be merged later to 9.4 builds.

Regards,
--
Michael

On 03/26/2014 08:39 AM, Michael Paquier wrote:
> Hi all,
>
> The behavior of rollback when an error occurs on an handle is
> controlled by extending Protocol with "$PROTNUM-[0|1|2]" where:
> - 0 = let the application handle rollbacks
> - 1 = rollback whole transaction when an error occurs
> - 2 = rollback only statement that failed
> Using such an extension is somewhat awkward as a single string is used
> for two settings... The proposed attached patch adds a new parameter
> called RollbackError that allows to control the behavior of rollback
> on error with a different parameter.

Great!

Since we're designing a new user interface for this, let's try to make
it as user-friendly as possible:

* I'm not too fond of the RollbackError name. It sounds like "an error
while rolling back". I googled around and found out that DataDirect's
proprietary driver has the same option, and they call it
TransactionErrorBehavior. See
http://blogs.datadirect.com/2013/07/solution-unexpected-postgres-current-transaction-aborted-error.html.

* Instead of using 0-2 as the values, let's give them descriptive names.
Something like "none", "RollbackTransaction", "RollbackStatement".
(actually, we'll probably want to also allow the integers, to keep the
connection string short, as there is a size limit on that)

> I thought first about including that in my cleanup work for 9.4, but
> as this actually does not break the driver it may be worth adding it
> directly to master, explaining the patch attached here. Comments
> welcome. Note that if there are objections I do not mind adding that
> for the work that would be merged later to 9.4 builds.

Yeah, let's get this into the master branch before your big 9.4 cleanup
work.

- Heikki

On Wed, Mar 26, 2014 at 3:39 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Hi all,
>
> The behavior of rollback when an error occurs on an handle is
> controlled by extending Protocol with "$PROTNUM-[0|1|2]"...
My apologies. This message was sent to the wrong mailing list and was
dedicated to odbc.
Once again sorry for that.
--
Michael

Hi Michael,

Isn't it an ODBC issue?

regards,
Hiroshi Inoue

(2014/03/26 15:39), Michael Paquier wrote:
> Hi all,
>
> The behavior of rollback when an error occurs on an handle is
> controlled by extending Protocol with "$PROTNUM-[0|1|2]" where:
> - 0 = let the application handle rollbacks
> - 1 = rollback whole transaction when an error occurs
> - 2 = rollback only statement that failed
> Using such an extension is somewhat awkward as a single string is used
> for two settings... The proposed attached patch adds a new parameter
> called RollbackError that allows to control the behavior of rollback
> on error with a different parameter.
>
> For backward-compatibility purposes, this patch does not break the old
> grammar of Protocol: it just gives the priority to RollbackError if
> both Protocol and RollbackError are set for a connection. Regression
> tests to test RollbackError and combinations of RollbackError/Protocol
> are added in the patch in the existing test error-rollback (which has
> needed some refactoring, older tests are not impacted). Docs are
> included as well.
>
> I thought first about including that in my cleanup work for 9.4, but
> as this actually does not break the driver it may be worth adding it
> directly to master, explaining the patch attached here. Comments
> welcome. Note that if there are objections I do not mind adding that
> for the work that would be merged later to 9.4 builds.
>
> Regards,

(Moving back this thread to pgsql-odbc, so keeping a maximum of message history)

On Wed, Mar 26, 2014 at 5:53 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 03/26/2014 08:39 AM, Michael Paquier wrote:
>> The behavior of rollback when an error occurs on an handle is
>> controlled by extending Protocol with "$PROTNUM-[0|1|2]" where:
>> - 0 = let the application handle rollbacks
>> - 1 = rollback whole transaction when an error occurs
>> - 2 = rollback only statement that failed
>> Using such an extension is somewhat awkward as a single string is used
>> for two settings... The proposed attached patch adds a new parameter
>> called RollbackError that allows to control the behavior of rollback
>> on error with a different parameter.
> Since we're designing a new user interface for this, let's try to make it as
> user-friendly as possible:
>
> * I'm not too fond of the RollbackError name. It sounds like "an error while
> rolling back". I googled around and found out that DataDirect's proprietary
> driver has the same option, and they call it TransactionErrorBehavior. See
> http://blogs.datadirect.com/2013/07/solution-unexpected-postgres-current-transaction-aborted-error.html.
Yeah, sounds fine... I am not coming with a better name now.

> * Instead of using 0-2 as the values, let's give them descriptive names.
> Something like "none", "RollbackTransaction", "RollbackStatement".
> (actually, we'll probably want to also allow the integers, to keep the
> connection string short, as there is a size limit on that)
Good idea. I'd vote for using only the strings in the new parameter
for simplicity, and to mention in the docs to which integer value maps
each string. We could as well use MEDIUM_REGISTRY_LEN (256 characters)
to control the size limit of the new parameter string.

>> I thought first about including that in my cleanup work for 9.4, but
>> as this actually does not break the driver it may be worth adding it
>> directly to master, explaining the patch attached here. Comments
>> welcome. Note that if there are objections I do not mind adding that
>> for the work that would be merged later to 9.4 builds.
>
> Yeah, let's get this into the master branch before your big 9.4 cleanup
> work.
OK, so I'll hack a patch following those lines. I shall be able to get
that soon with regression tests.
Regards,
--
Michael

(2014/03/27 20:55), Michael Paquier wrote:
> (Moving back this thread to pgsql-odbc, so keeping a maximum of message history)

As for the patch in the original post,

Is there any difference between rollback_on_error and
rollback_error_value except the naming?
The length of connection strings seem to be increased at least 3 bytes.
Is it right?

regards,
Hiroshi Inoue

--
I am using the free version of SPAMfighter.
SPAMfighter has removed 7015 of my spam emails to date.
Get the free SPAMfighter here: http://www.spamfighter.com/len

Do you have a slow PC? Try a Free scan
http://www.spamfighter.com/SLOW-PCfighter?cid=sigen

On Fri, Mar 28, 2014 at 12:53 PM, Inoue, Hiroshi <inoue(at)tpf(dot)co(dot)jp> wrote:
> (2014/03/27 20:55), Michael Paquier wrote:
>>
>> (Moving back this thread to pgsql-odbc, so keeping a maximum of message
>> history)
>
>
> As for the patch in the original post,
>
> Is there any difference between rollback_on_error and
> rollback_error_value except the naming?
No differences. I just changed the name of this variable to be a
maximum consistent with the parameter introduced.

> The length of connection strings seem to be increased at least 3 bytes.
> Is it right?
Do you mean that with the introduction of the new parameter AA? If
yes, well I guess it is increased by 5 bytes ";AA=[0|1|2]".
--
Michael

(2014/03/28 14:37), Michael Paquier wrote:
> On Fri, Mar 28, 2014 at 12:53 PM, Inoue, Hiroshi <inoue(at)tpf(dot)co(dot)jp> wrote:
>> (2014/03/27 20:55), Michael Paquier wrote:
>>>
>>> (Moving back this thread to pgsql-odbc, so keeping a maximum of message
>>> history)
>>
>>
>> As for the patch in the original post,
>>
>> Is there any difference between rollback_on_error and
>> rollback_error_value except the naming?
> No differences. I just changed the name of this variable to be a
> maximum consistent with the parameter introduced.

There are some rollback_on_error in dlg_wingui.c.

>> The length of connection strings seem to be increased at least 3 bytes.
>> Is it right?
> Do you mean that with the introduction of the new parameter AA? If
> yes, well I guess it is increased by 5 bytes ";AA=[0|1|2]".

Isn't the difference between
A1=7.4;AA=[0|1|2] and A1=7.4-[0|1|2]
?
I introduced the latter only to skimp on 3 bytes.

regards,
Hiroshi Inoue

On Sat, Mar 29, 2014 at 7:16 AM, Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
> (2014/03/28 14:37), Michael Paquier wrote:
>>
>> On Fri, Mar 28, 2014 at 12:53 PM, Inoue, Hiroshi <inoue(at)tpf(dot)co(dot)jp> wrote:
>> No differences. I just changed the name of this variable to be a
>> maximum consistent with the parameter introduced.
>
>
> There are some rollback_on_error in dlg_wingui.c.
Ouch, thanks.

>>> The length of connection strings seem to be increased at least 3 bytes.
>>> Is it right?
>>
>> Do you mean that with the introduction of the new parameter AA? If
>> yes, well I guess it is increased by 5 bytes ";AA=[0|1|2]".
>
>
> Isn't the difference between
> A1=7.4;AA=[0|1|2] and A1=7.4-[0|1|2]
> ?
> I introduced the latter only to skimp on 3 bytes.
Oh yes, sorry. Is it a problem though? Because if we switch to
string-only values for AA this could get longer by 10~20 bytes
depending on the parameter values.
--
Michael

(2014/03/29 7:23), Michael Paquier wrote:
> On Sat, Mar 29, 2014 at 7:16 AM, Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
>> (2014/03/28 14:37), Michael Paquier wrote:
>>>
>>> On Fri, Mar 28, 2014 at 12:53 PM, Inoue, Hiroshi <inoue(at)tpf(dot)co(dot)jp> wrote:
>>> No differences. I just changed the name of this variable to be a
>>> maximum consistent with the parameter introduced.
>>
>>
>> There are some rollback_on_error in dlg_wingui.c.
> Ouch, thanks.
>
>>>> The length of connection strings seem to be increased at least 3 bytes.
>>>> Is it right?
>>>
>>> Do you mean that with the introduction of the new parameter AA? If
>>> yes, well I guess it is increased by 5 bytes ";AA=[0|1|2]".
>>
>>
>> Isn't the difference between
>> A1=7.4;AA=[0|1|2] and A1=7.4-[0|1|2]
>> ?
>> I introduced the latter only to skimp on 3 bytes.
> Oh yes, sorry. Is it a problem though? Because if we switch to
> string-only values for AA this could get longer by 10~20 bytes
> depending on the parameter values.

SQLDriverConnect() returns the complete connection strings when
requested. makeConnectString() in dlg_specific.c makes connection
strings which are acceptable for specified buffers.

regards,
Hiroshi Inoue

On Wed, Mar 26, 2014 at 5:53 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> * I'm not too fond of the RollbackError name. It sounds like "an error while
> rolling back". I googled around and found out that DataDirect's proprietary
> driver has the same option, and they call it TransactionErrorBehavior. See
> http://blogs.datadirect.com/2013/07/solution-unexpected-postgres-current-transaction-aborted-error.html.
>
> * Instead of using 0-2 as the values, let's give them descriptive names.
> Something like "none", "RollbackTransaction", "RollbackStatement".
> (actually, we'll probably want to also allow the integers, to keep the
> connection string short, as there is a size limit on that)
OK, I have been working more on that, giving the attached patch. The
parameter name is changed to TransactionErrorBehavior, able to use the
values "Statement", "Nop", "Transaction" and "Default". "Default"
corresponds to the default behavior, that is used to set the rollback
behavior depending on the Postgres version driver is connected with.
As of now, TransactionErrorBehavior does not accept integer values as
it makes the patch a bit more simple, this could be added with some
atoi calls in dlg_specific.c.

I have updated dlg_wingui.c as well. Patch has always its set of docs
and regression tests.

Regards,
--
Michael

On Mon, Mar 31, 2014 at 9:40 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> OK, I have been working more on that, giving the attached patch...
Sorry, wrong mailing list...
My apologies.
--
Michael

Unfortunately I sent this patch to hackers once again... For ODBC
users please use the patch on this email.

> * I'm not too fond of the RollbackError name. It sounds like "an error while
> rolling back". I googled around and found out that DataDirect's proprietary
> driver has the same option, and they call it TransactionErrorBehavior. See
> http://blogs.datadirect.com/2013/07/solution-unexpected-postgres-current-transaction-aborted-error.html.
>
> * Instead of using 0-2 as the values, let's give them descriptive names.
> Something like "none", "RollbackTransaction", "RollbackStatement".
> (actually, we'll probably want to also allow the integers, to keep the
> connection string short, as there is a size limit on that)

I have been working more on that, resulting in the patch attached. The
parameter name is changed to TransactionErrorBehavior, able to use the
values "Statement", "Nop", "Transaction" and "Default". "Default"
corresponds to the default behavior, that is used to set the rollback
behavior depending on the Postgres version driver is connected with.
As of now, TransactionErrorBehavior does not accept integer values as
it makes the patch a bit more simple, this could be added with some
atoi calls in dlg_specific.c.

I have updated dlg_wingui.c as well. Patch has always its set of docs
and regression tests.

On Sat, Mar 29, 2014 at 7:16 AM, Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
> There are some rollback_on_error in dlg_wingui.c.
Corrected in the patch attached.

Regards,
--
Michael