# Ticket #7204 (assigned defect)

Opened 3 years ago

## Avoid db.rollback() in upgrade_environment()

Reported by: Assigned to: pipern rjollos (accepted) normal TracPastePlugin major AttributeError upgrade db hasienda, lkraav 0.12

### Description

db.rollback() should be avoided in upgrade_environment(db), as this would rollback the actions of other plugins which are also upgrading at the same time.

I don't know if it's the best way - for now, I just commented out the db.rollback(). I'm not certain if it's needed after a SELECT throws an exception due to missing table.

## Change History

### 12/29/11 21:43:20 changed by ejucovy

• severity changed from normal to major.

This fix is necessary for the plugin to work on a fresh installation of Trac 0.12 and above, because the database connection that's used in the needs_upgrade step is a read-only connection. So the db.rollback() fails, resulting (mysteriously) in an AttributeError when running trac-admin env upgrade, and the system never notices it needs an upgrade.

### 06/24/12 23:24:59 changed by rjollos

• owner changed from otaku42 to rjollos.
• cc set to hasienda, lkraav.
• status changed from new to assigned.

On a fresh install of the plugin in 1.0dev @ t:r11053, I didn't see the upgrade database request message, and when navigating to the Pastebin tab, I saw the following error:

Trac detected an internal error:

OperationalError: no such table: pastes


This is consistent with comment:1 's observations, I think. I'm not to the point yet that I feel comfortable with the implementing and manipulating IEnvironmentSetupParticipant, but since this is a blocker and the issue seems to be fixed by removing the db.rollback()s, I'm going to apply the fix. I also wonder whether if it is really necessary or desirable to wrap all of the DB queries with try/excepts.

Code review and comments are welcome and appreciated!

### 06/24/12 23:31:24 changed by rjollos

(In [11687]) Refs #7204: Attempt to introduce Trac 0.12 compatibility by removing db.rollback()s, which are thought to present a problem because the DB connection is read-only.

Tested with Trac 0.13dev (1.0dev) @ t:r11053.

### 06/24/12 23:35:34 changed by rjollos

I've been using FullBlogPlugin as a guide in implementing several features. I wouldn't be surprised if the original author had been looking at FullBlogPlugin as well, a lot of the code looks very similar.

FullBlogPlugin doesn't have a single db.rollback in fullblogplugin/0.11/tracfullblog/db.py.

### 06/24/12 23:45:49 changed by ejucovy

FWIW, I've been happily running this plugin (http://trac-hacks.org/svn/tracpasteplugin/0.11@r10450) on my Trac-dev environment (http://svn.edgewall.org/repos/trac/trunk@r10896) for ~6 months with only one of the db.rollback() lines removed:

Index: tracpaste/db.py
===================================================================
--- tracpaste/db.py	(revision 11062)
+++ tracpaste/db.py	(working copy)
@@ -93,7 +93,6 @@
row = cursor.fetchone()
return row and 1 or 0
except:
-            db.rollback()
return 0

return 0


That said, I think it makes sense to remove all the db.rollback()s. I also think it would probably be reasonable to remove all those try .. except blocks -- Trac should be trusted to handle unexpected exceptions in plugin code. This level of error handling in the plugin will just bury the errors.

### 06/25/12 20:51:53 changed by hasienda

• keywords set to AttributeError upgrade db.

See t:#10451 for the original issue reported for Trac.

In short: For plugin db schema version checking TagsPlugin resorted to testing for table existence instead of using canonical entries in Trac db table system. And some others re-used this approach too. This was not very smart, but worked; until the Trac db API change in t:changeset:10179, where the .rollback() method has been removed consequently from all read-only connections. And this applies i.e. to the connection, that is handed over in the method environment_needs_upgrade() to all classes implementing IEnvironmentSetupParticipant.

So the best way is to switch over to the aforementioned explicit version number storage. A temporary work-around is proposed i.e. in changeset:11041 to resolve #9521 reported against TagsPlugin. You'll find it quite similar to this ticket - for a reason.

### 07/18/12 00:27:48 changed by rjollos

I'm still not entirely understanding when I should be using rollbacks and handling them as in [11041], but removing them seems to have helped with #10156.

I will revisit this soon and try to better understand the issue.

### 09/29/12 15:09:45 changed by hasienda

The issue has been discussed in #9521 and further in #5552, and what I meant by 'explicit' plugin db schema version numbering is implemented now for TagsPlugin in [12069].

### Add/Change #7204 (Avoid db.rollback() in upgrade_environment())

Change Properties