Re: rollback to savepoint leads to transaction already in progress

Hi all,

Trivial program to demonstrate problem:

main() {
ECPGdebug(1,stderr);
exec sql connect to postgres;
exec sql set autocommit to off;
exec sql start transaction;
exec sql savepoint s;
exec sql rollback to s;
exec sql commit;
return 0;
}

Output:

[28397]: ECPGdebug: set to 1
[28397]: ECPGconnect: opening database postgres on <DEFAULT> port <DEFAULT>
[28397]: ECPGsetcommit on line 4: action "off"; connection "postgres"
[28397]: ECPGtrans on line 5: action "start transaction"; connection
"postgres"
[28397]: ECPGtrans on line 6: action "savepoint s"; connection "postgres"
[28397]: ECPGtrans on line 7: action "rollback to s"; connection "postgres"
[28397]: ECPGtrans on line 8: action "commit"; connection "postgres"
[28397]: ECPGnoticeReceiver: there is already a transaction in progress
[28397]: raising sqlcode -603

Problem:

It shouldn't raise "there is already a transaction in progress" error,
particularly when doing a commit. Remove "rollback to s" and no problem.

Environment:

ecpg (PostgreSQL 8.4.5) 4.5.0
Linux thales 2.6.32-24-generic #43-Ubuntu SMP Thu Sep 16 14:58:24 UTC
2010 x86_64 GNU/Linux
Two cores

This appears to be a regression; it doesn't occur with ecpg (PostgreSQL
8.3.8) 4.4.1

Regards,

David

On Mon, Oct 11, 2010 at 7:42 AM, David Newall
<postgresql(at)davidnewall(dot)com> wrote:
> Trivial program to demonstrate problem:
>
> main() {
>    ECPGdebug(1,stderr);
>    exec sql connect to postgres;
>    exec sql set autocommit to off;
>    exec sql start transaction;
>    exec sql savepoint s;
>    exec sql rollback to s;
>    exec sql commit;
>    return 0;
> }
>
> Output:
> [28397]: ECPGdebug: set to 1
> [28397]: ECPGconnect: opening database postgres on <DEFAULT> port <DEFAULT>
> [28397]: ECPGsetcommit on line 4: action "off"; connection "postgres"
> [28397]: ECPGtrans on line 5: action "start transaction"; connection "postgres"
> [28397]: ECPGtrans on line 6: action "savepoint s"; connection "postgres"
> [28397]: ECPGtrans on line 7: action "rollback to s"; connection "postgres"
> [28397]: ECPGtrans on line 8: action "commit"; connection "postgres"
> [28397]: ECPGnoticeReceiver: there is already a transaction in progress
> [28397]: raising sqlcode -603
>
> Problem:
> It shouldn't raise "there is already a transaction in progress" error,
> particularly when doing a commit.  Remove "rollback to s" and no problem.
>
> Environment:
> ecpg (PostgreSQL 8.4.5) 4.5.0
> This appears to be a regression; it doesn't occur with ecpg (PostgreSQL
> 8.3.8) 4.4.1

The bug comes from string-based transaction control in ECPGtrans().
The code cannot distinguish ROLLBACK TRANSACTION and ROLLBACK TO savepoint.
----
if (strncmp(transaction, "commit", 6) == 0 || strncmp(transaction,
"rollback", 8) == 0)
con->committed = true;
else
con->committed = false;
----

I think the string-comparison is unreliable. So, I'd like to replace
the code to use PQtransactionStatus(). I have two patches to do it:
The first one (ecpg-trans-quick_20101014.patch) is a quick fix
that replaces only the above test.
The second one (ecpg-trans-full_20101014.patch) replaces all of
struct connection->committed with PQtransactionStatus().

Which solution is better? Or, another idea?

--
Itagaki Takahiro

On 14/10/10 12:14, Itagaki Takahiro wrote:
> Which solution is better? Or, another idea?
>

This does seem to be an new bug in previously working code. While any
solution that fixes the problem is good, it might pay to look the code
that worked before. As reported, it worked for ecpg (PostgreSQL 8.3.8)
4.4.1.

On Thu, Oct 14, 2010 at 3:52 PM, David Newall
<postgresql(at)davidnewall(dot)com> wrote:
> This does seem to be an new bug in previously working code.  While any
> solution that fixes the problem is good, it might pay to look the code that
> worked before.  As reported, it worked for ecpg (PostgreSQL 8.3.8) 4.4.1.

It works on 8.3, but it's still broken. Here is the code in 8.3.
It ignores "ROLLBACK TO savepoint", but also ignores "ROLLBACK TRANSACTION".

----
if (strcmp(transaction, "commit") == 0 || strcmp(transaction, "rollback") == 0)
con->committed = true;
else
con->committed = false;
----

--
Itagaki Takahiro

Am 14.10.2010 08:52, schrieb David Newall:
> On 14/10/10 12:14, Itagaki Takahiro wrote:
>> Which solution is better? Or, another idea?
>
> This does seem to be an new bug in previously working code. While any
> solution that fixes the problem is good, it might pay to look the code
> that worked before. As reported, it worked for ecpg (PostgreSQL 8.3.8)
> 4.4.1.

You seem to be answering to an email that I didn't see and couldn't find
in the archive either. Was it send in private or to the list? If it went
to the list I might be lagging behind.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at googlemail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

On Thu, Oct 14, 2010 at 6:53 AM, Michael Meskes <meskes(at)postgresql(dot)org> wrote:
> Am 14.10.2010 08:52, schrieb David Newall:
>>
>> On 14/10/10 12:14, Itagaki Takahiro wrote:
>>>
>>> Which solution is better? Or, another idea?
>>
>> This does seem to be an new bug in previously working code. While any
>> solution that fixes the problem is good, it might pay to look the code
>> that worked before. As reported, it worked for ecpg (PostgreSQL 8.3.8)
>> 4.4.1.
>
> You seem to be answering to an email that I didn't see and couldn't find in
> the archive either. Was it send in private or to the list? If it went to the
> list I might be lagging behind.

I didn't get it either. The original report was posted to pgsql-bugs
on October 10.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Takahiro-san,

> I think the string-comparison is unreliable. So, I'd like to replace
> the code to use PQtransactionStatus(). I have two patches to do it:
> The first one (ecpg-trans-quick_20101014.patch) is a quick fix
> that replaces only the above test.
> The second one (ecpg-trans-full_20101014.patch) replaces all of
> struct connection->committed with PQtransactionStatus().
>
> Which solution is better? Or, another idea?

The full solution is the better one, but I'd prefer to not apply it to
the stable versions as there's a slight chance it might break something.
So that means I'd apply full for HEAD and quick for 9.0 and 8.4. Will
you commit this? Or shall I?

Michael

--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at googlemail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

On Thu, Oct 14, 2010 at 6:53 AM, Michael Meskes<meskes(at)postgresql(dot)org> wrote:

> You seem to be answering to an email that I didn't see and couldn't find in
> the archive either. Was it send in private or to the list? If it went to the
> list I might be lagging behind.
>

On 14/10/10 22:51, Robert Haas wrote:
> I didn't get it either. The original report was posted to pgsql-bugs
> on October 10.
>

It might be lag; it'll probably turn up, about 6 seconds after I press *S

-------- Original Message --------
Subject: Re: [BUGS] rollback to savepoint leads to transaction already
in progress
Date: Thu, 14 Oct 2010 10:44:50 +0900
From: Itagaki Takahiro<itagaki(dot)takahiro(at)gmail(dot)com>
To: David Newall<postgresql(at)davidnewall(dot)com>, PostgreSQL Hackers
<pgsql-hackers(at)postgresql(dot)org>
CC: pgsql-bugs(at)postgresql(dot)org

On Mon, Oct 11, 2010 at 7:42 AM, David Newall
<postgresql(at)davidnewall(dot)com> wrote:
> Trivial program to demonstrate problem:
>
> main() {
> ECPGdebug(1,stderr);
> exec sql connect to postgres;
> exec sql set autocommit to off;
> exec sql start transaction;
> exec sql savepoint s;
> exec sql rollback to s;
> exec sql commit;
> return 0;
> }
>
> Output:
> [28397]: ECPGdebug: set to 1
> [28397]: ECPGconnect: opening database postgres on<DEFAULT> port<DEFAULT>
> [28397]: ECPGsetcommit on line 4: action "off"; connection "postgres"
> [28397]: ECPGtrans on line 5: action "start transaction"; connection "postgres"
> [28397]: ECPGtrans on line 6: action "savepoint s"; connection "postgres"
> [28397]: ECPGtrans on line 7: action "rollback to s"; connection "postgres"
> [28397]: ECPGtrans on line 8: action "commit"; connection "postgres"
> [28397]: ECPGnoticeReceiver: there is already a transaction in progress
> [28397]: raising sqlcode -603
>
> Problem:
> It shouldn't raise "there is already a transaction in progress" error,
> particularly when doing a commit. Remove "rollback to s" and no problem.
>
> Environment:
> ecpg (PostgreSQL 8.4.5) 4.5.0
> This appears to be a regression; it doesn't occur with ecpg (PostgreSQL
> 8.3.8) 4.4.1

The bug comes from string-based transaction control in ECPGtrans().
The code cannot distinguish ROLLBACK TRANSACTION and ROLLBACK TO savepoint.
----
if (strncmp(transaction, "commit", 6) == 0 || strncmp(transaction,
"rollback", 8) == 0)
con->committed = true;
else
con->committed = false;
----

I think the string-comparison is unreliable. So, I'd like to replace
the code to use PQtransactionStatus(). I have two patches to do it:
The first one (ecpg-trans-quick_20101014.patch) is a quick fix
that replaces only the above test.
The second one (ecpg-trans-full_20101014.patch) replaces all of
struct connection->committed with PQtransactionStatus().

Which solution is better? Or, another idea?

--
Itagaki Takahiro