Skip Menu |

This queue is for tickets about the DBD-Pg CPAN distribution.

Report information
The Basics
Id: 57695
Status: rejected
Priority: 0/
Queue: DBD-Pg

People
Owner: Nobody in particular
Requestors: SAMV [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Unimportant
Broken in: 2.17.1
Fixed in: (no value)



Subject: DBD::Pg unconditionally rolling back destroyed statements on destroy (in pg_st_deallocate_statement)
Hi, I'm not sure why you choose to rollback when a statement handle is destroyed. This breaks the idea that the transaction should stay in the failed state until explicitly rolled back or committed. If you were, say, to then explicitly roll back to a named savepoint, it would roll back to the previous savepoint of the same name. The rollback on DBH disconnect behaviour is fine, that is fair enough, but given that statement handles may be created temporarily and then thrown away, this behaviour is a bad default IMHO.
On Fri May 21 01:19:32 2010, SAMV wrote: Show quoted text
> Hi, I'm not sure why you choose to rollback when a statement handle is > destroyed. This breaks the idea that the transaction should stay in > the failed state until explicitly rolled back or committed.
I suggest the attached patch, unless someone can describe why this is required at all.
Subject: no-rollback-on-st-destroy.patch
--- libdbd-pg-perl-2.8.7.orig/dbdimp.c 2008-07-24 07:46:49.000000000 +1200 +++ libdbd-pg-perl-2.8.7/dbdimp.c 2010-05-24 11:41:19.000000000 +1200 @@ -3414,42 +3414,6 @@ tempsqlstate[0] = '\0'; - /* What is our status? */ - tstatus = pg_db_txn_status(aTHX_ imp_dbh); - if (TRACE5) - TRC(DBILOGFP, "%stxn_status is %d\n", THEADER, tstatus); - - /* If we are in a failed transaction, rollback before deallocating */ - if (PQTRANS_INERROR == tstatus) { - if (TRACE4) - TRC(DBILOGFP, "%sIssuing rollback before deallocate\n", THEADER); - { - /* If a savepoint has been set, rollback to the last savepoint instead of the entire transaction */ - I32 alen = av_len(imp_dbh->savepoints); - if (alen > -1) { - char *cmd; - SV * const sp = *av_fetch(imp_dbh->savepoints, alen, 0); - New(0, cmd, SvLEN(sp) + 13, char); /* Freed below */ - if (TRACE4) - TRC(DBILOGFP, "%sRolling back to savepoint %s\n", THEADER, SvPV_nolen(sp)); - sprintf(cmd, "rollback to %s", SvPV_nolen(sp)); - strncpy(tempsqlstate, imp_dbh->sqlstate, strlen(imp_dbh->sqlstate)+1); - status = _result(aTHX_ imp_dbh, cmd); - Safefree(cmd); - } - else { - status = _result(aTHX_ imp_dbh, "ROLLBACK"); - imp_dbh->done_begin = DBDPG_FALSE; - } - } - if (PGRES_COMMAND_OK != status) { - /* This is not fatal, it just means we cannot deallocate */ - if (TRACEWARN) TRC(DBILOGFP, "%sRollback failed, so no deallocate\n", THEADER); - if (TEND) TRC(DBILOGFP, "%sEnd pg_st_deallocate_statement (cannot deallocate)\n", THEADER); - return 1; - } - } - New(0, stmt, strlen("DEALLOCATE ") + strlen(imp_sth->prepare_name) + 1, char); /* freed below */ sprintf(stmt, "DEALLOCATE %s", imp_sth->prepare_name);
On Sun May 23 19:59:11 2010, SAMV wrote: Show quoted text
> On Fri May 21 01:19:32 2010, SAMV wrote:
> > Hi, I'm not sure why you choose to rollback when a statement handle
is Show quoted text
> > destroyed. This breaks the idea that the transaction should stay in > > the failed state until explicitly rolled back or committed.
> > I suggest the attached patch, unless someone can describe why this is > required at all.
Right, I now see. Postgres refuses to deal even with non-data related operations such as DEALLOCATE or PREPARE in this state. Yuck. Still, implicit rollback is very evil, a more correct solution would defer the work which will not be accepted until the transaction is in a valid state again.
On Mon May 24 20:28:19 2010, SAMV wrote: Show quoted text
> On Sun May 23 19:59:11 2010, SAMV wrote:
> > On Fri May 21 01:19:32 2010, SAMV wrote:
> > > Hi, I'm not sure why you choose to rollback when a statement
handle Show quoted text
> is
> > > destroyed. This breaks the idea that the transaction should stay
in Show quoted text
> > > the failed state until explicitly rolled back or committed.
> > > > I suggest the attached patch, unless someone can describe why this
is Show quoted text
> > required at all.
> > Right, I now see. Postgres refuses to deal even with non-data related > operations such as DEALLOCATE or PREPARE in this state. Yuck. Still, > implicit rollback is very evil, a more correct solution would defer
the Show quoted text
> work which will not be accepted until the transaction is in a valid
state Show quoted text
> again.
OK, after a bit of investigation I'm more convinced that the code is unnecessary. In the event that an $sth goes out of scope during an aborted transaction, the DEALLOCATE statement will simply fail, a warning will be issued by the enclosing code, but nothing disastrous should result of this. Even if you explicitly re-used a prepared statement name using pg_prepare_name, it would fail, and then pick a different name. I'll see if I can prove this with a test case.
On Mon May 24 20:46:50 2010, SAMV wrote: Show quoted text
> OK, after a bit of investigation I'm more convinced that the code is > unnecessary. In the event that an $sth goes out of scope during an > aborted transaction, the DEALLOCATE statement will simply fail, a > warning will be issued by the enclosing code, but nothing disastrous > should result of this. Even if you explicitly re-used a prepared > statement name using pg_prepare_name, it would fail, and then pick a > different name. I'll see if I can prove this with a test case.
Continuing my monologue, after producing a test case (attached) I can see that the exact drawback I describe is not correct, leaving me with just the general grumble that the driver is resetting my transaction state without telling me.
Subject: 20savepoints.t
#!perl ## Test savepoint functionality use 5.006; use strict; use warnings; use Test::More; use DBI ':sql_types'; use lib 't','.'; require 'dbdpg_test_setup.pl'; select(($|=1,select(STDERR),$|=1)[1]); my $dbh = connect_database(); if (!defined $dbh) { plan skip_all => 'Connection to database failed, cannot continue testing'; } plan "no_plan"; isnt ($dbh, undef, 'Connect to database for savepoint testing'); my $pgversion = $dbh->{pg_server_version}; my $t; my $str = 'Savepoint Test'; my $ids = sub { my $ids = $dbh->selectcol_arrayref('SELECT id FROM dbd_pg_test WHERE pname = ? order by id asc',undef,$str); return $ids; }; SKIP: { skip ('Cannot test savepoints on pre-8.0 servers', 2) if $pgversion < 80000; my $sth = $dbh->prepare('INSERT INTO dbd_pg_test (id,pname) VALUES (?,?)'); ## Create 500 without a savepoint $sth->execute(500,$str); ## Create 501 inside a savepoint and roll it back $dbh->pg_savepoint('dbd_pg_test_savepoint'); $sth->execute(501,$str); $dbh->pg_rollback_to('dbd_pg_test_savepoint'); ## savepoints can be re-used. Make 505 and roll it back. $sth->execute(502,$str); $dbh->pg_rollback_to('dbd_pg_test_savepoint'); ## Create 502 after the rollback: $sth->execute(503,$str); $dbh->commit; $t='pg_savepoint and pg_rollback_to - re-using a savepoint'; is_deeply($ids->(), [500, 503], $t); ## however, re-using a savepoint name also stacks them. $dbh->pg_savepoint('dbd_pg_test_savepoint'); $sth->execute(504,$str); $dbh->pg_savepoint('dbd_pg_test_savepoint'); $sth->execute(505,$str); $dbh->pg_rollback_to('dbd_pg_test_savepoint'); $dbh->commit; $t='duplicate savepoint name behaviour'; is_deeply($ids->(), [500, 503, 504], $t); ## we can also 'release' savepoints $dbh->pg_savepoint('dbd_pg_test_savepoint'); $sth->execute(506,$str); $dbh->pg_release('dbd_pg_test_savepoint'); # these ones should fail eval { $dbh->pg_rollback_to('dbd_pg_test_savepoint') }; $t = "exception by rollback to released savepoint"; like($@, qr/no such savepoint/, $t); eval { $sth->execute(507,$str) }; $t = "exception from trying to execute stuff"; like($@, qr/current.*aborted/, $t); ## this will be a forced rollback. $dbh->commit; $t = 'pg_release to release savepoint'; is_deeply($ids->(), [500, 503, 504], $t); ## test rollback with an actual exception $dbh->pg_savepoint('dbd_pg_test_savepoint'); $sth->execute(508,$str); $dbh->pg_savepoint('dbd_pg_test_savepoint'); $sth->execute(509,$str); # we do something bad, eval { $dbh->selectall_arrayref('gibbons') }; # say we detect the error and roll back, $dbh->pg_rollback_to('dbd_pg_test_savepoint'); $sth->execute(510,$str); $dbh->commit; $t = 'no implicit rollback on statement deallocate'; is_deeply($ids->(), [500, 503, 504, 508, 510], $t); # test what happens if you re-use savepoints and roll back twice $dbh->do('SAVEPOINT dbd_pg_test_savepoint'); $sth->execute(511,$str); $dbh->do('SAVEPOINT dbd_pg_test_savepoint'); $sth->execute(512,$str); # we do something bad, eval { $dbh->selectall_arrayref('gibbons') }; # the statement handle rolls back for us, $dbh->do('ROLLBACK TO dbd_pg_test_savepoint'); # we detect the error and roll back as before, $dbh->do('ROLLBACK TO dbd_pg_test_savepoint'); $sth->execute(513,$str); $dbh->commit; # ah, it's safe - because the first rollback didn't *release* # the savepoint. $t = 'rollback to savepoint - re-using rollbacks in the presence of stacking'; is_deeply($ids->(), [500, 503, 504, 508, 510, 511, 513], $t); } $dbh->do('DELETE FROM dbd_pg_test'); $dbh->commit(); cleanup_database($dbh,'test'); $dbh->disconnect();
Show quoted text
> the general grumble that the driver is resetting my transaction state > without telling me.
So do you want to close this bug? If not, I'd suggest you start a discussion on the dbdpg mailing list.
Stalling ticket for now
Closing this as "rejected", but feel free to reopen. But the lack of anyone else complaining limits the chances we will address this without at least some discussion on the list.