Modify

Opened 14 years ago

Closed 9 years ago

#7204 closed defect (fixed)

Avoid db.rollback() in upgrade_environment()

Reported by: pipern Owned by: Ryan J Ollos
Priority: normal Component: TracPastePlugin
Severity: major Keywords: AttributeError upgrade db
Cc: Steffen Hoffmann, lkraav Trac Release: 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.

Attachments (0)

Change History (10)

comment:1 Changed 13 years ago by ejucovy

Severity: normalmajor

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.

comment:2 Changed 12 years ago by Ryan J Ollos

Cc: Steffen Hoffmann lkraav added; anonymous removed
Owner: changed from Michael Renzmann to Ryan J Ollos
Status: newassigned

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!

comment:3 Changed 12 years ago by Ryan J Ollos

(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.

comment:4 Changed 12 years ago by Ryan J Ollos

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.

comment:5 Changed 12 years ago 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:

  • tracpaste/db.py

     
    9393            row = cursor.fetchone()
    9494            return row and 1 or 0
    9595        except:
    96             db.rollback()
    9796            return 0
    9897
    9998        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.

comment:6 Changed 12 years ago by Steffen Hoffmann

Keywords: AttributeError upgrade db added

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.

comment:7 Changed 12 years ago by Ryan J Ollos

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.

comment:8 Changed 12 years ago by Steffen Hoffmann

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].

comment:9 Changed 11 years ago by Ryan J Ollos

Status: assignednew

comment:10 Changed 9 years ago by Ryan J Ollos

Resolution: fixed
Status: newclosed

In 14798:

0.3.0dev: Adapt to Trac 1.1.6dev API. Fixes #5658, #7204.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.