Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#8625 closed defect (fixed)

Drop SQLite2 support

Reported by: anatoly techtonik <techtonik@…> Owned by: Remy Blank
Priority: normal Milestone: 0.12
Component: database backend Version: 0.11-stable
Severity: normal Keywords: pysqlite
Cc: Branch:
Release Notes:
API Changes:

Description

When trying to run 0.11.x Trac on an old sqlite2 DB the following error is displayed.

Traceback (most recent call last):
  File "m:\p\trac\trac-0.11dev\trac\web\api.py", line 377, in send_error
    'text/html')
  File "m:\p\trac\trac-0.11dev\trac\web\chrome.py", line 727, in render_template
    req.chrome[type_].append(
  File "m:\p\trac\trac-0.11dev\trac\web\api.py", line 195, in __getattr__
    value = self.callbacks[name](self)
  File "m:\p\trac\trac-0.11dev\trac\web\chrome.py", line 488, in prepare_request
    for category, name, text in contributor.get_navigation_items(req):
  File "m:\p\trac\trac-0.11dev\trac\ticket\web_ui.py", line 163, in get_navigation_items
    if 'TICKET_CREATE' in req.perm:
  File "m:\p\trac\trac-0.11dev\trac\perm.py", line 527, in has_permission
    return self._has_permission(action, resource)
  File "m:\p\trac\trac-0.11dev\trac\perm.py", line 541, in _has_permission
    check_permission(action, perm.username, resource, perm)
  File "m:\p\trac\trac-0.11dev\trac\perm.py", line 428, in check_permission
    perm)
  File "m:\p\trac\trac-0.11dev\trac\perm.py", line 285, in check_permission
    get_user_permissions(username)
  File "m:\p\trac\trac-0.11dev\trac\perm.py", line 360, in get_user_permissions
    for perm in self.store.get_user_permissions(username):
  File "m:\p\trac\trac-0.11dev\trac\perm.py", line 178, in get_user_permissions
    cursor.execute("SELECT username,action FROM permission")
  File "m:\p\trac\trac-0.11dev\trac\db\util.py", line 60, in execute
    return self.cursor.execute(sql)
  File "m:\p\trac\trac-0.11dev\trac\db\sqlite_backend.py", line 58, in execute
    args or [])
  File "m:\p\trac\trac-0.11dev\trac\db\sqlite_backend.py", line 50, in _rollback_on_error
    return function(self, *args, **kwargs)
DatabaseError: file is encrypted or is not a database

The proposal is to drop support for SQLite2 format and display message with a link to convert instructions when an old format is detected. http://trac.edgewall.org/wiki/PySqlite#TheSQLitelibrary

See also Debian bug: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=501338

Attachments (3)

sqlite-get_supported_schemes (2.0 KB ) - added by Christian Boos 10 years ago.
Better reporting of installation issues related to PySqlite.
sqlite-get_supported_schemes.2 (2.9 KB ) - added by Christian Boos 10 years ago.
new version of the patch, more elaborate version checks for PySqlite and simplified DatabaseManager._get_connector
8625-get-supported-schemes-r8575.patch (7.3 KB ) - added by Remy Blank 10 years ago.
Updated patch, adds support for errors in the other backends.

Download all attachments as: .zip

Change History (20)

comment:1 by Christian Boos, 10 years ago

Keywords: pysqlite added
Milestone: 0.12
Owner: set to Christian Boos

Well, that error simply means that you're not accessing a SQLite v3 database, this could be a SQLite v2 file, or anything else.

I'm not sure it's worth making a special case for this error (or we could as well do so for the other errors listed in PySqlite#CommonOops).

That being said, dropping the support for PySqlite 1.0.x was on my TODO list for 0.12. I think there's no need for a deprecation period, as hardly anyone would be using it for new installations or even upgrades.

comment:2 by anatoly techtonik <techtonik@…>, 10 years ago

I agree that deprecation is not needed, but there are still many installations of Trac 0.9 and even 0.8, so some kind of transition is still needed.

I would like to create warning on Trac 0.11.x if the old format of SQLite DB is still in use.

comment:3 by Christian Boos, 10 years ago

OK, I'll do that for the upcoming 0.11.6.

comment:4 by Remy Blank, 10 years ago

Owner: changed from Christian Boos to Remy Blank

A quick question: currently, the SQLite backend always reports that it supports the sqlite scheme, even if the bindings could not be imported. Shouldn't get_supported_schemes() check if the bindings are available, and only return the supported scheme if they are?

The MySQL backend does that check, the PostgreSQL doesn't.

comment:5 by Remy Blank, 10 years ago

Support for PySqlite 1.0.x was removed in [8571].

in reply to:  4 comment:6 by Christian Boos, 10 years ago

Resolution: fixed
Status: newclosed

Replying to rblank: And deprecation warning for 0.11.6 added in [8573].

A quick question: currently, the SQLite backend always reports that it supports the sqlite scheme, even if the bindings could not be imported. Shouldn't get_supported_schemes() check if the bindings are available, and only return the supported scheme if they are?

He, and that's just another patch I had in preparation (for #8562), where I did something along the lines of what we do for the version control connectors. See on that ticket.

by Christian Boos, 10 years ago

Better reporting of installation issues related to PySqlite.

comment:7 by Christian Boos, 10 years ago

On second thought, the patch is better placed here, as the #8562 ticket is a bit confusing.

comment:8 by Remy Blank, 10 years ago

I was thinking much simpler:

  • trac/db/sqlite_backend.py

    diff --git a/trac/db/sqlite_backend.py b/trac/db/sqlite_backend.py
    a b  
    9090        self._version = None
    9191
    9292    def get_supported_schemes(self):
    93         return [('sqlite', 1)]
     93        if have_pysqlite:
     94            return [('sqlite', 1)]
     95        return []
    9496
    9597    def get_connection(self, path, log=None, params={}):
    9698        if not self._version:

It's always the same error anyway: the scheme is not supported by the current configuration, which is always due to missing bindings. Are you sure the extra "complexity" is worth it?

comment:9 by Remy Blank, 10 years ago

Oh, and BTW, you should check for priority < 0 outside of the loop, otherwise a backend reporting an error could "shadow" a working backend for the same scheme.

Do you want me to apply the patch, and propagate the changes to the other backends?

comment:10 by Remy Blank, 10 years ago

Resolution: fixed
Status: closedreopened

in reply to:  8 comment:11 by Christian Boos, 10 years ago

Replying to rblank:

I was thinking much simpler: … It's always the same error anyway: the scheme is not supported by the current configuration, which is always due to missing bindings.

We could also report when some known bad SQLite/PySqlite version is detected. For example, we should do so for PySqlite versions < 1.0.7 and < 2.0.7, which have known issues with SQLite ≥ 3.3.3. Also, we could "blacklist" problematic versions (like 2.5.0) instead of having workaround code for it. For each of these case (or future ones), we could have an informative message.

Are you sure the extra "complexity" is worth it?

It's the simplest way I found to propagate the error message up. It's not that complex, and we reuse a pattern already used for the vc connectors, so nothing fundamentally new.

by Christian Boos, 10 years ago

new version of the patch, more elaborate version checks for PySqlite and simplified DatabaseManager._get_connector

comment:12 by anatoly techtonik <techtonik@…>, 10 years ago

As this one is initially my bug, I would like to record my vision. Unfortunately, I wasn't able to provide a patch, because of #8647 that took quite a lot of my time, but here is what I've tried:

  • Add IEnvironmentSetupParticipant to SQLiteConnector

Failed, because SQLiteConnector component is called after Environment component and the latter already uses SQL queries in participant.environment_needs_upgrade(db) calls.

10:16:02 PM Trac[env] ERROR: Exception caught while checking for upgrade: 
Traceback (most recent call last):
  File "m:\p\trac\trac-0.11dev\trac\env.py", line 586, in open_environment
    needs_upgrade = env.needs_upgrade()
  File "m:\p\trac\trac-0.11dev\trac\env.py", line 429, in needs_upgrade
    if participant.environment_needs_upgrade(db):
  File "m:\p\trac\trac-0.11dev\trac\env.py", line 500, in environment_needs_upgrade
    dbver = self.env.get_version(db)
  File "m:\p\trac\trac-0.11dev\trac\env.py", line 351, in get_version
    (initial and 'initial_' or ''))
  File "m:\p\trac\trac-0.11dev\trac\db\util.py", line 60, in execute
    return self.cursor.execute(sql)
...

Of course if DB format is wrong there is no way for cursor.execute() to return correctly.

  • Next I thought that if Environment class is called first - it should call the DB backend to figure out if it needs upgrade.
Index: trac/env.py
===================================================================
--- trac/env.py	(revision 8555)
+++ trac/env.py	(working copy)
@@ -497,6 +497,8 @@
         self._update_sample_config()
 
     def environment_needs_upgrade(self, db):
+        if db.db_needs_upgrade():
+            raise TracError(_('Database needs upgrade'))
         dbver = self.env.get_version(db)
         if dbver == db_default.db_version:
             return False
  • And while trying to figure out where to plug this db_needs_upgrade() I've stuck with #8647

What we need (IMHO) is basically a big warning message in 0.11.6 if user still uses SQLite 2.x database with a link to instructions how to install proper bindings and upgrade the DB (in Debian I thought to provide a script to do the upgrade semi-automatically).

Let me present the other scenario - what happens if a fresh Trac installation is supplied with old environment and SQLite 2.x bindings are not present. Then there will be exception mentioned in this ticket description. It should be intercepted and before it is passed further DB should be checked for an old format by reading first 48 bytes from file and matching them against ** This file contains an SQLite 2.1 database ** string. I.e. do not propose to install old SQLite bindings, but urge people to upgrade to newer version instead, listing actual bugs for older version if possible. That was my original idea how to make this properly.

by Remy Blank, 10 years ago

Updated patch, adds support for errors in the other backends.

comment:13 by Remy Blank, 10 years ago

The patch above reworks attachment:sqlite-get_supported_schemes.2 slightly, and adds support for the new way of enumerating connectors to the PostgreSQL and MySQL backends.

Ok to apply to 0.11-stable and merge to trunk? I suppose that is what was intended, as the patch was against 0.11-stable.

comment:14 by Christian Boos, 10 years ago

Yes, I didn't test it, but looks good, so I'm OK if you apply it (yes, 0.11-stable was the target).

comment:15 by Remy Blank, 10 years ago

Resolution: fixed
Status: reopenedclosed

Patch applied in [8582].

comment:16 by anatoly techtonik <techtonik@…>, 10 years ago

What about more prominent message about the necessity to upgrade SQLite 2.x database? The one that will be visible to everyone and not only in Trac logs?

According to PySqlite page: "It should still be possible to use the old SQLite 2.8.x version: everything seems to work (except the search, see #2960) You should then use an equally old Pysqlite version, namely 1.0.1. But doing so is really not recommended, you'll get awful performances and frequent "database is locked" errors (see #7785, #3446)."

It seems that upgrading to SQLite 3.x is something people should have been done even before 0.11.x

in reply to:  16 comment:17 by Christian Boos, 10 years ago

Replying to anatoly techtonik <techtonik@…>:

What about more prominent message about the necessity to upgrade SQLite 2.x database? The one that will be visible to everyone and not only in Trac logs?

Well, I think the deprecation warning in 0.11.6 and the removal in 0.12 is more than enough. But we should take care of saying a word about this in the 0.11.6 announcement as well.

According to PySqlite page:

I'll update that.

It seems that upgrading to SQLite 3.x is something people should have been done even before 0.11.x

Certainly ;-)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.