Merge lp:~jelmer/bzr-svn/sqlite-lock into lp:bzr-svn/1.1

[Jelmer Vernooij]

When hitting a SQLite lock, simply continue without the cache.

[Michael Hudson-Doyle (mwhudson)]

This looks ok to me. (dunno if you wanted my opinion :p). I bet you didn't mean to leave that "raise ImportError" in though :)

The change in get_revision_paths looks a little strange -- does what happens in the else: clause of the try: not access the db?

[Jelmer Vernooij (jelmer)]

Thanks for the review, and catching that ImportError. Removed now :)

=== modified file 'NEWS'
--- NEWS	2011-08-09 15:07:13 +0000
+++ NEWS	2011-08-15 17:39:32 +0000
@@ -96,6 +96,9 @@
   * Fix importing of non-ascii characters in bzr:revprop: values.
     (Jelmer Vernooij, #821168)
 
+  * Allow concurrent access of SQLite cache, rather than aborting.
+    (Jelmer Vernooij, #685251)
+
  FEATURES
 
   * When roundtripping revisions a revision property with the Bazaar testament is

=== modified file 'cache/__init__.py'
--- cache/__init__.py	2011-08-13 14:22:02 +0000
+++ cache/__init__.py	2011-08-15 17:39:32 +0000
@@ -38,6 +38,10 @@
 from bzrlib.plugins.svn import version_info
 
 
+class CacheConcurrencyError(Exception):
+    """Unable to access cache while write is in progress."""
+
+
 def write_cache_readme(path):
     f = open(path, 'w')
     try:
@@ -146,6 +150,7 @@
 try:
     from bzrlib.plugins.svn.cache.tdbcache import TdbRepositoryCache
     cache_cls = TdbRepositoryCache
+    raise ImportError
 except ImportError:
     from bzrlib.plugins.svn.cache.sqlitecache import SqliteRepositoryCache
     cache_cls = SqliteRepositoryCache

=== modified file 'cache/sqlitecache.py'
--- cache/sqlitecache.py	2011-07-30 16:44:57 +0000
+++ cache/sqlitecache.py	2011-08-15 17:39:32 +0000
@@ -27,6 +27,7 @@
     )
 
 from bzrlib.plugins.svn.cache import (
+    CacheConcurrencyError,
     RepositoryCache,
     )
 from bzrlib.plugins.svn.mapping import (
@@ -71,7 +72,7 @@
 
 
 def _connect_sqlite3_file(path):
-    return sqlite3.connect(path, timeout=20.0)
+    return sqlite3.connect(path, timeout=20.0, isolation_level=None)
 
 
 connect_cachefile = _connect_sqlite3_file
@@ -87,8 +88,32 @@
             self.cachedb = cache_db
         self._commit_interval = 500
         self._create_table()
-        self.cachedb.commit()
-        self._commit_countdown = self._commit_interval
+        self.commit()
+
+    @classmethod
+    def _convert_operational_errors(cls, e):
+        if "database is locked" in e:
+            return CacheConcurrencyError()
+        else:
+            return e
+
+    def execute(self, *args, **kwargs):
+        try:
+            return self.cachedb.execute(*args, **kwargs)
+        except sqlite3.OperationalError, e:
+            raise self._convert_operational_errors(e)
+
+    def executemany(self, *args, **kwargs):
+        try:
+            return self.cachedb.executemany(*args, **kwargs)
+        except sqlite3.OperationalError, e:
+            raise self._convert_operational_errors(e)
+
+    def executescript(self, *args, **kwargs):
+        try:
+            return self.cachedb.executescript(*args, **kwargs)
+        except sqlite3.OperationalError, e:
+            raise self._convert_operational_errors(e)
 
     def commit(self):
         """Commit the changes to the database."""
@@ -140,13 +165,13 @@
         """See RevisionIdMapCache.set_last_revnum_checked."""
 
         self.mutter("set last revnum checked for %r to %r", layout, revnum)
-        self.cachedb.execute("replace into revids_seen (layout, max_revnum) VALUES (?, ?)", (layout, revnum))
+        self.execute("replace into revids_seen (layout, max_revnum) VALUES (?, ?)", (layout, revnum))
         self.commit()
 
     def last_revnum_checked(self, layout):
         """See RevisionIdMapCache.last_revnum_checked."""
 
-        ret = self.cachedb.execute(
+        ret = self.execute(
             "select max_revnum from revids_seen where layout = ?", (layout,)).fetchone()
         if ret is None:
             revnum = 0
@@ -160,7 +185,7 @@
 
         assert isinstance(revid, str)
         self.mutter("lookup revid %r", revid)
-        ret = self.cachedb.execute(
+        ret = self.execute(
             "select path, min_revnum, max_revnum, mapping from revmap where revid=? order by abs(min_revnum-max_revnum) asc", (revid,)).fetchone()
         if ret is None:
             raise errors.NoSuchRevision(self, revid)
@@ -176,7 +201,7 @@
         assert isinstance(revnum, int)
         assert isinstance(path, str)
         assert isinstance(mapping, str)
-        row = self.cachedb.execute(
+        row = self.execute(
                 "select revid from revmap where max_revnum=? and min_revnum=? and path=? and mapping=?", (revnum, revnum, path, mapping)).fetchone()
         if row is not None:
             ret = str(row[0])
@@ -195,16 +220,16 @@
         assert min_revnum <= max_revnum
         self.mutter("insert revid %r:%r-%r -> %r", branch, min_revnum, max_revnum, revid)
         if min_revnum == max_revnum:
-            cursor = self.cachedb.execute(
+            cursor = self.execute(
                 "update revmap set min_revnum = ?, max_revnum = ? WHERE revid=? AND path=? AND mapping=?",
                 (min_revnum, max_revnum, revid, branch, mapping))
         else:
-            cursor = self.cachedb.execute(
+            cursor = self.execute(
                 "update revmap set min_revnum = MAX(min_revnum,?), max_revnum = MIN(max_revnum, ?) WHERE revid=? AND path=? AND mapping=?",
                 (min_revnum, max_revnum, revid, branch, mapping))
         if cursor.rowcount == 0:
-            self.cachedb.execute(
-                "insert into revmap (revid,path,min_revnum,max_revnum,mapping) VALUES (?,?,?,?,?)",
+            self.execute(
+                "replace into revmap (revid,path,min_revnum,max_revnum,mapping) VALUES (?,?,?,?,?)",
                 (revid, branch, min_revnum, max_revnum, mapping))
         self._commit_conditionally()
 
@@ -212,7 +237,7 @@
 class SqliteRevisionInfoCache(RevisionInfoCache, CacheTable):
 
     def _create_table(self):
-        self.cachedb.executescript("""
+        self.executescript("""
         create table if not exists revmetainfo (
             path text not null,
             revnum integer,
@@ -240,20 +265,20 @@
             orig_mapping_name = original_mapping.name
         else:
             orig_mapping_name = None
-        self.cachedb.execute("insert into original_mapping (path, revnum, original_mapping) values (?, ?, ?)", (foreign_revid[1].decode("utf-8"), foreign_revid[2], orig_mapping_name))
+        self.execute("replace into original_mapping (path, revnum, original_mapping) values (?, ?, ?)", (foreign_revid[1].decode("utf-8"), foreign_revid[2], orig_mapping_name))
 
     def insert_revision(self, foreign_revid, mapping, (revno, revid, hidden),
             stored_lhs_parent_revid):
         """See RevisionInfoCache.insert_revision."""
 
-        self.cachedb.execute("insert into revmetainfo (path, revnum, mapping, revid, revno, hidden, stored_lhs_parent_revid) values (?, ?, ?, ?, ?, ?, ?)", (foreign_revid[1], foreign_revid[2], mapping.name, revid, revno, hidden, stored_lhs_parent_revid))
+        self.execute("replace into revmetainfo (path, revnum, mapping, revid, revno, hidden, stored_lhs_parent_revid) values (?, ?, ?, ?, ?, ?, ?)", (foreign_revid[1], foreign_revid[2], mapping.name, revid, revno, hidden, stored_lhs_parent_revid))
         self._commit_conditionally()
 
     def get_revision(self, foreign_revid, mapping):
         """See RevisionInfoCache.get_revision."""
 
         # Will raise KeyError if not present
-        row = self.cachedb.execute("select revno, revid, hidden, stored_lhs_parent_revid from revmetainfo where path = ? and revnum = ? and mapping = ?", (foreign_revid[1], foreign_revid[2], mapping.name)).fetchone()
+        row = self.execute("select revno, revid, hidden, stored_lhs_parent_revid from revmetainfo where path = ? and revnum = ? and mapping = ?", (foreign_revid[1], foreign_revid[2], mapping.name)).fetchone()
         if row is None:
             raise KeyError((foreign_revid, mapping))
         else:
@@ -270,7 +295,7 @@
     def get_original_mapping(self, foreign_revid):
         """See RevisionInfoCache.get_original_mapping."""
 
-        row = self.cachedb.execute("select original_mapping from original_mapping where path = ? and revnum = ?", (foreign_revid[1].decode("utf-8"), foreign_revid[2])).fetchone()
+        row = self.execute("select original_mapping from original_mapping where path = ? and revnum = ?", (foreign_revid[1].decode("utf-8"), foreign_revid[2])).fetchone()
         if row is None:
             raise KeyError(foreign_revid)
         if row[0] is None:
@@ -282,7 +307,7 @@
 class SqliteLogCache(LogCache, CacheTable):
 
     def _create_table(self):
-        self.cachedb.executescript("""
+        self.executescript("""
             create table if not exists changed_path(
                 rev integer,
                 action text not null check(action in ('A', 'D', 'M', 'R')),
@@ -305,7 +330,7 @@
             create unique index if not exists revinfo_rev on revinfo(rev);
         """ % NODE_UNKNOWN)
         try:
-            self.cachedb.executescript(
+            self.executescript(
                 "alter table changed_path ADD kind INT DEFAULT %d;" % NODE_UNKNOWN)
         except sqlite3.OperationalError:
             pass # Column already exists.
@@ -314,8 +339,8 @@
         """See LogCache.find_latest_change."""
 
         if path == "":
-            return self.cachedb.execute("select max(rev) from changed_path where rev <= ?", (revnum,)).fetchone()[0]
-        return self.cachedb.execute("""
+            return self.execute("select max(rev) from changed_path where rev <= ?", (revnum,)).fetchone()[0]
+        return self.execute("""
             select max(rev) from changed_path
             where rev <= ?
             and (path=?
@@ -327,7 +352,7 @@
     def get_revision_paths(self, revnum):
         """See LogCache.get_revision_paths."""
 
-        result = self.cachedb.execute("select path, action, copyfrom_path, copyfrom_rev, kind from changed_path where rev=?", (revnum,))
+        result = self.execute("select path, action, copyfrom_path, copyfrom_rev, kind from changed_path where rev=?", (revnum,))
         paths = {}
         for p, act, cf, cr, kind in result:
             if cf is not None:
@@ -353,19 +378,19 @@
                 kind = NODE_UNKNOWN
             new_paths.append((rev, p.strip("/").decode("utf-8"), v[0], copyfrom_path, v[2], kind))
 
-        self.cachedb.executemany("replace into changed_path (rev, path, action, copyfrom_path, copyfrom_rev, kind) values (?, ?, ?, ?, ?, ?)", new_paths)
+        self.executemany("replace into changed_path (rev, path, action, copyfrom_path, copyfrom_rev, kind) values (?, ?, ?, ?, ?, ?)", new_paths)
 
     def drop_revprops(self, revnum):
         """See LogCache.drop_revprops."""
 
-        self.cachedb.execute("update revinfo set all_revprops = 0 where rev = ?", (revnum,))
+        self.execute("update revinfo set all_revprops = 0 where rev = ?", (revnum,))
 
     def get_revprops(self, revnum):
         """See LogCache.get_revprops."""
 
-        result = self.cachedb.execute("select name, value from revprop where rev = ?", (revnum,))
+        result = self.execute("select name, value from revprop where rev = ?", (revnum,))
         revprops = dict((k.encode("utf-8"), v.encode("utf-8")) for (k, v) in result)
-        result = self.cachedb.execute("select all_revprops from revinfo where rev = ?", (revnum,)).fetchone()
+        result = self.execute("select all_revprops from revinfo where rev = ?", (revnum,)).fetchone()
         if result is None:
             all_revprops = False
         else:
@@ -377,15 +402,15 @@
 
         if revprops is None:
             return
-        self.cachedb.executemany("replace into revprop (rev, name, value) values (?, ?, ?)", [(revision, name.decode("utf-8", "replace"), value.decode("utf-8", "replace")) for (name, value) in revprops.iteritems()])
-        self.cachedb.execute("""
+        self.executemany("replace into revprop (rev, name, value) values (?, ?, ?)", [(revision, name.decode("utf-8", "replace"), value.decode("utf-8", "replace")) for (name, value) in revprops.iteritems()])
+        self.execute("""
             replace into revinfo (rev, all_revprops) values (?, ?)
         """, (revision, all_revprops))
 
     def max_revnum(self):
         """See LogCache.last_revnum."""
 
-        saved_revnum = self.cachedb.execute("SELECT MAX(rev) FROM changed_path").fetchone()[0]
+        saved_revnum = self.execute("SELECT MAX(rev) FROM changed_path").fetchone()[0]
         if saved_revnum is None:
             return 0
         return saved_revnum
@@ -393,13 +418,13 @@
     def min_revnum(self):
         """See LogCache.min_revnum."""
 
-        return self.cachedb.execute("SELECT MIN(rev) FROM changed_path").fetchone()[0]
+        return self.execute("SELECT MIN(rev) FROM changed_path").fetchone()[0]
 
 
 class SqliteParentsCache(ParentsCache, CacheTable):
 
     def _create_table(self):
-        self.cachedb.executescript("""
+        self.executescript("""
         create table if not exists parent (
             rev text not null,
             parent text,
@@ -415,17 +440,17 @@
 
         self.mutter('insert parents: %r -> %r', revid, parents)
         if len(parents) == 0:
-            self.cachedb.execute("replace into parent (rev, parent, idx) values (?, NULL, -1)", (revid,))
+            self.execute("replace into parent (rev, parent, idx) values (?, NULL, -1)", (revid,))
         else:
             for i, p in enumerate(parents):
-                self.cachedb.execute("replace into parent (rev, parent, idx) values (?, ?, ?)", (revid, p, i))
+                self.execute("replace into parent (rev, parent, idx) values (?, ?, ?)", (revid, p, i))
         self._commit_conditionally()
 
     def lookup_parents(self, revid):
         """See ParentsCache.lookup_parents."""
 
         self.mutter('lookup parents: %r', revid)
-        rows = self.cachedb.execute("select parent from parent where rev = ? order by idx", (revid, )).fetchall()
+        rows = self.execute("select parent from parent where rev = ? order by idx", (revid, )).fetchall()
         if len(rows) == 0:
             return None
         return tuple([row[0].encode("utf-8") for row in rows if row[0] is not None])

=== modified file 'logwalker.py'
--- logwalker.py	2011-08-15 01:13:33 +0000
+++ logwalker.py	2011-08-15 17:39:32 +0000
@@ -33,6 +33,9 @@
 from bzrlib.plugins.svn import (
     changes,
     )
+from bzrlib.plugins.svn.cache import (
+    CacheConcurrencyError,
+    )
 from bzrlib.plugins.svn.transport import (
     SvnRaTransport,
     )
@@ -231,6 +234,9 @@
         if "logwalker" in debug.debug_flags:
             trace.mutter(text, *args, **kwargs)
 
+    def _warn_busy_cache(self):
+        trace.warning("Cache for repository %s busy, ignoring")
+
     def find_latest_change(self, path, revnum):
         """Find latest revision that touched path.
 
@@ -239,7 +245,11 @@
         """
         assert isinstance(path, str)
         assert isinstance(revnum, int) and revnum >= 0
-        self._fetch_revisions(revnum)
+        try:
+            self._fetch_revisions(revnum)
+        except CacheConcurrencyError:
+            self._warn_busy_cache()
+            return self.actual.find_latest_change(path, revnum)
 
         self.mutter("latest change: %r:%r", path, revnum)
         try:
@@ -263,19 +273,33 @@
         """
         assert from_revnum >= 0 and to_revnum >= 0, "%r -> %r" % (from_revnum, to_revnum)
         self.mutter("iter changes %r->%r (%r)", from_revnum, to_revnum, prefixes)
-        self._fetch_revisions(max(from_revnum, to_revnum), pb=pb)
+        try:
+            self._fetch_revisions(max(from_revnum, to_revnum), pb=pb)
+        except CacheConcurrencyError:
+            self._warn_busy_cache()
+            return self.actual.iter_changes(prefixes, from_revnum, to_revnum,
+                    limit, pb)
         return iter(iter_changes(prefixes, from_revnum, to_revnum,
             self.get_revision_paths, self.revprop_list, limit))
 
     def get_revision_paths(self, revnum):
         if revnum == 0:
             return changes.REV0_CHANGES
-        self._fetch_revisions(revnum)
-        return self.cache.get_revision_paths(revnum)
+        try:
+            self._fetch_revisions(revnum)
+        except CacheConcurrencyError:
+            self._warn_busy_cache()
+            return self.actual.get_revision_paths(revnum)
+        else:
+            return self.cache.get_revision_paths(revnum)
 
     def revprop_list(self, revnum):
         self.mutter('revprop list: %d' % revnum)
-        self._fetch_revisions(revnum)
+        try:
+            self._fetch_revisions(revnum)
+        except CacheConcurrencyError:
+            self._warn_busy_cache()
+            return self.actual.revprop_list(revnum)
 
         if revnum > 0:
             (known_revprops, has_all_revprops) = self.cache.get_revprops(revnum)
@@ -290,8 +314,11 @@
 
     def _caching_revprop_list(self, revnum):
         revprops = self._transport.revprop_list(revnum)
-        self.cache.insert_revprops(revnum, revprops, True)
-        self.cache.commit()
+        try:
+            self.cache.insert_revprops(revnum, revprops, True)
+            self.cache.commit()
+        except CacheConcurrencyError:
+            pass
         return revprops
 
     def _fetch_revisions(self, to_revnum, pb=None):

=== modified file 'revmeta.py'
--- revmeta.py	2011-08-13 11:27:34 +0000
+++ revmeta.py	2011-08-15 17:39:32 +0000
@@ -35,6 +35,9 @@
     errors as svn_errors,
     util,
     )
+from bzrlib.plugins.svn.cache import (
+    CacheConcurrencyError,
+    )
 from bzrlib.plugins.svn.mapping import (
     SVN_PROP_BZR_HIDDEN,
     SVN_PROP_BZR_REVPROP_REDIRECT,
@@ -735,14 +738,19 @@
         self._stored_lhs_parent_revid = {}
 
     def _update_cache(self, mapping):
-        if (self.get_original_mapping() is not None and
-            self._revision_info[mapping][1] is not None):
-            self._revid_cache.insert_revid(self._revision_info[mapping][1],
-                self.metarev.branch_path, self.metarev.revnum,
-                self.metarev.revnum, mapping.name)
-        self._revinfo_cache.insert_revision(self.metarev.get_foreign_revid(), mapping,
-            self._revision_info[mapping],
-            self._stored_lhs_parent_revid[mapping])
+        try:
+            if (self.get_original_mapping() is not None and
+                self._revision_info[mapping][1] is not None):
+                self._revid_cache.insert_revid(self._revision_info[mapping][1],
+                    self.metarev.branch_path, self.metarev.revnum,
+                    self.metarev.revnum, mapping.name)
+            self._revinfo_cache.insert_revision(
+                self.metarev.get_foreign_revid(),
+                mapping,
+                self._revision_info[mapping],
+                self._stored_lhs_parent_revid[mapping])
+        except CacheConcurrencyError:
+            pass
 
     def _determine(self, mapping):
         self._revision_info[mapping] = self.base.get_revision_info(mapping)
@@ -752,8 +760,8 @@
         assert mapping is not None
         (self._revision_info[mapping],
                 self._stored_lhs_parent_revid[mapping]) = \
-                 self._revinfo_cache.get_revision(self.metarev.get_foreign_revid(),
-                                                  mapping)
+                 self._revinfo_cache.get_revision(
+                         self.metarev.get_foreign_revid(), mapping)
 
     def get_original_mapping(self):
         if self._original_mapping_set:
@@ -763,8 +771,12 @@
                 self.metarev.get_foreign_revid())
         except KeyError:
             self._original_mapping = self.base.get_original_mapping()
-            self._revinfo_cache.set_original_mapping(self.metarev.get_foreign_revid(),
+            try:
+                self._revinfo_cache.set_original_mapping(
+                    self.metarev.get_foreign_revid(),
                     self._original_mapping)
+            except CacheConcurrencyError:
+                pass
         self._original_mapping_set = True
         return self._original_mapping
 
@@ -815,7 +827,10 @@
             if parent_ids is not None:
                 return parent_ids
         parent_ids = self.base.get_parent_ids(mapping, parentrevmeta)
-        self._parents_cache.insert_parents(myrevid, parent_ids)
+        try:
+            self._parents_cache.insert_parents(myrevid, parent_ids)
+        except CacheConcurrencyError:
+            pass
         return parent_ids