Modify

Opened 4 years ago

Last modified 14 months ago

#7204 new defect

Avoid db.rollback() in upgrade_environment()

Reported by: pipern Owned by: rjollos
Priority: normal Component: TracPastePlugin
Severity: major Keywords: AttributeError upgrade db
Cc: hasienda, 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 (9)

comment:1 Changed 3 years ago 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.

comment:2 Changed 2 years ago by rjollos

  • Cc hasienda lkraav added
  • Owner changed from otaku42 to rjollos
  • 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!

comment:3 Changed 2 years ago 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.

comment:4 Changed 2 years ago 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.

comment:5 Changed 2 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 2 years ago by hasienda

  • 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 2 years ago 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.

comment:8 Changed 23 months ago 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].

comment:9 Changed 14 months ago by rjollos

  • Status changed from assigned to new

Add Comment

Modify Ticket

Action
as new .
Author


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

 
Note: See TracTickets for help on using tickets.