Fix race condition for key properties in rdbms backends [SF#1876683]

Author: Richard Jones

CHANGES.txt

@@ -1,7 +1,7 @@
 This file contains the changes to the Roundup system over time. The entries
 are given with the most recent entry first.
 
-2007-??-?? 1.4.2
+2008-02-07 1.4.2
 Feature:
 - New config option in mail section: ignore_alternatives allows to
   ignore alternatives besides the text/plain part used for the content
@@ -16,10 +16,10 @@ Feature:
 Fixed:
 - Searching date range by supplying just a date as the filter spec
 - Handle no time.tzset under Windows (sf #1825643)
-- Work around race condition in file storage during transaction commit
-  (sf #1883580)
+- Fix race condition in file storage transaction commit (sf #1883580)
 - Make user utils JS work with firstname/lastname again (sf #1868323)
 - Fix ZRoundup to work with Zope 2.8.5 (sf #1806125)
+- Fix race condition for key properties in rdbms backends (sf #1876683)
 
 
 2007-11-09 1.4.1

doc/admin_guide.txt

@@ -2,7 +2,7 @@
 Administration Guide
 ====================
 
-:Version: $Revision: 1.26 $
+:Version: $Revision: 1.27 $
 
 .. contents::
 
@@ -209,6 +209,12 @@ take:
 4. Stop the tracker web and email frontends.
 5. Follow the steps in the `upgrading documentation`_ for the new version of
    the software in the copied.
+
+   Usually you will be asked to run `roundup_admin migrate` on your tracker
+   before you allow users to start accessing the tracker.
+
+   It's safe to run this even if it's not required, so just get into the
+   habit.
 6. You may test each of the admin tool, web interface and mail gateway using
    the new version of the software. To do this, invoke the scripts directly
    in the source directory with::

doc/upgrading.txt

@@ -13,6 +13,24 @@ steps.
 
 .. contents::
 
+Migrating from 1.4.x to 1.4.2
+=============================
+
+You should run the "roundup-admin migrate" command for your tracker once
+you've installed the latest codebase. 
+
+Do this before you use the web, command-line or mail interface and before
+any users access the tracker.
+
+This command will respond with either "Tracker updated" (if you've not
+previously run it on an RDBMS backend) or "No migration action required"
+(if you have run it, or have used another interface to the tracker,
+or are using anydbm).
+
+It's safe to run this even if it's not required, so just get into the
+habit.
+
+
 Migrating from 1.3.3 to 1.4.0
 =============================
 
@@ -25,12 +43,12 @@ update the messagesummary detector as follows::
     --- detectors/messagesummary.py 17 Apr 2003 03:26:38 -0000      1.1
     +++ detectors/messagesummary.py 3 Apr 2007 06:47:21 -0000       1.2
     @@ -8,7 +8,7 @@
-	 if newvalues.has_key('summary') or not newvalues.has_key('content'):
-	     return
+     if newvalues.has_key('summary') or not newvalues.has_key('content'):
+         return
 
     -    summary, content = parseContent(newvalues['content'], 1, 1)
     +    summary, content = parseContent(newvalues['content'], config=db.config)
-	 newvalues['summary'] = summary
+     newvalues['summary'] = summary
 
 In the latest version we have added some database indexes to the
 SQL-backends (mysql, postgresql, sqlite) for speeding up building the

roundup/admin.py

@@ -16,7 +16,7 @@
 # BASIS, AND THERE IS NO OBLIGATION WHATSOEVER TO PROVIDE MAINTENANCE,
 # SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.
 #
-# $Id: admin.py,v 1.109 2007-09-26 14:09:59 jpend Exp $
+# $Id: admin.py,v 1.110 2008-02-07 03:28:33 richard Exp $
 
 '''Administration commands for maintaining Roundup trackers.
 '''
@@ -1333,6 +1333,33 @@ def do_security(self, args):
                     print _(' %(description)s (%(name)s)')%d
         return 0
 
+
+    def do_migrate(self, args):
+        '''Usage: migrate
+        Update a tracker's database to be compatible with the Roundup
+        codebase.
+
+        You should run the "migrate" command for your tracker once you've
+        installed the latest codebase. 
+
+        Do this before you use the web, command-line or mail interface and
+        before any users access the tracker.
+
+        This command will respond with either "Tracker updated" (if you've
+        not previously run it on an RDBMS backend) or "No migration action
+        required" (if you have run it, or have used another interface to the
+        tracker, or possibly because you are using anydbm).
+
+        It's safe to run this even if it's not required, so just get into
+        the habit.
+        '''
+        if getattr(self.db, 'db_version_updated'):
+            print _('Tracker updated')
+            self.db_uncommitted = True
+        else:
+            print _('No migration action required')
+        return 0
+
     def run_command(self, args):
         '''Run a single command
         '''

roundup/backends/rdbms_common.py

@@ -15,7 +15,7 @@
 # BASIS, AND THERE IS NO OBLIGATION WHATSOEVER TO PROVIDE MAINTENANCE,
 # SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.
 #
-#$Id: rdbms_common.py,v 1.193 2007-10-25 07:26:11 richard Exp $
+#$Id: rdbms_common.py,v 1.194 2008-02-07 03:28:34 richard Exp $
 """ Relational database (SQL) backend common code.
 
 Basics:
@@ -29,7 +29,7 @@
 - journals are stored adjunct to the per-class tables
 - table names and columns have "_" prepended so the names can't clash with
   restricted names (like "order")
-- retirement is determined by the __retired__ column being true
+- retirement is determined by the __retired__ column being > 0
 
 Database-specific changes may generally be pushed out to the overridable
 sql_* methods, since everything else should be fairly generic. There's
@@ -42,6 +42,13 @@
 that maps to a table. If that information differs from the hyperdb schema,
 then we update it. We also store in the schema dict a version which
 allows us to upgrade the database schema when necessary. See upgrade_db().
+
+To force a unqiueness constraint on the key properties we put the item
+id into the __retired__ column duing retirement (so it's 0 for "active"
+items) and place a unqiueness constraint on key + __retired__. This is
+particularly important for the users class where multiple users may
+try to have the same username, with potentially many retired users with
+the same name.
 """
 __docformat__ = 'restructuredtext'
 
@@ -239,7 +246,8 @@ def post_init(self):
 
     # update this number when we need to make changes to the SQL structure
     # of the backen database
-    current_db_version = 4
+    current_db_version = 5
+    db_version_updated = False
     def upgrade_db(self):
         """ Update the SQL database to reflect changes in the backend code.
 
@@ -272,7 +280,11 @@ def upgrade_db(self):
         if version < 4:
             self.fix_version_3_tables()
 
+        if version < 5:
+            self.fix_version_4_tables()
+
         self.database_schema['version'] = self.current_db_version
+        self.db_version_updated = True
         return 1
 
     def fix_version_3_tables(self):
@@ -283,9 +295,21 @@ def fix_version_3_tables(self):
             self.sql('ALTER TABLE %ss ADD %s_value TEXT'%(name, name))
 
     def fix_version_2_tables(self):
-        """Default (used by sqlite): NOOP"""
+        # Default (used by sqlite): NOOP
         pass
 
+    def fix_version_4_tables(self):
+        # note this is an explicit call now
+        c = self.cursor
+        for cn, klass in self.classes.items():
+            c.execute('select id from _%s where __retired__<>0'%(cn,))
+            for (id,) in c.fetchall():
+                c.execute('update _%s set __retired__=%s where id=%s'%(cn,
+                    self.arg, self.arg), (id, id))
+
+            if klass.key:
+                self.add_class_key_required_unique_constraint(cn, klass.key)
+
     def _convert_journal_tables(self):
         """Get current journal table contents, drop the table and re-create"""
         c = self.cursor
@@ -530,9 +554,18 @@ def create_class_table_indexes(self, spec):
                         spec.classname, spec.key)
             self.sql(index_sql3)
 
+            # and the unique index for key / retired(id)
+            self.add_class_key_required_unique_constraint(spec.classname,
+                spec.key)
+
         # TODO: create indexes on (selected?) Link property columns, as
         # they're more likely to be used for lookup
 
+    def add_class_key_required_unique_constraint(self, cn, key):
+        sql = '''create unique index _%s_key_retired_idx 
+            on _%s(__retired__, _%s)'''%(cn, cn, key)
+        self.sql(sql)
+
     def drop_class_table_indexes(self, cn, key):
         # drop the old table indexes first
         l = ['_%s_id_idx'%cn, '_%s_retired_idx'%cn]
@@ -555,10 +588,15 @@ def create_class_table_key_index(self, cn, key):
     def drop_class_table_key_index(self, cn, key):
         table_name = '_%s'%cn
         index_name = '_%s_%s_idx'%(cn, key)
-        if not self.sql_index_exists(table_name, index_name):
-            return
-        sql = 'drop index '+index_name
-        self.sql(sql)
+        if self.sql_index_exists(table_name, index_name):
+            sql = 'drop index '+index_name
+            self.sql(sql)
+
+        # and now the retired unique index too
+        index_name = '_%s_key_retired_idx'%cn
+        if self.sql_index_exists(table_name, index_name):
+            sql = 'drop index _%s_key_retired_idx'%cn
+            self.sql(sql)
 
     def create_journal_table(self, spec):
         """ create the journal table for a class given the spec and
@@ -1760,7 +1798,7 @@ def retire(self, nodeid):
         # conversion (hello, sqlite)
         sql = 'update _%s set __retired__=%s where id=%s'%(self.classname,
             self.db.arg, self.db.arg)
-        self.db.sql(sql, (1, nodeid))
+        self.db.sql(sql, (nodeid, nodeid))
         if self.do_journal:
             self.db.addjournal(self.classname, nodeid, ''"retired", None)
 
@@ -1802,7 +1840,7 @@ def is_retired(self, nodeid):
         sql = 'select __retired__ from _%s where id=%s'%(self.classname,
             self.db.arg)
         self.db.sql(sql, (nodeid,))
-        return int(self.db.sql_fetchone()[0])
+        return int(self.db.sql_fetchone()[0]) > 0
 
     def destroy(self, nodeid):
         """Destroy a node.
@@ -1880,9 +1918,9 @@ def lookup(self, keyvalue):
 
         # use the arg to handle any odd database type conversion (hello,
         # sqlite)
-        sql = "select id from _%s where _%s=%s and __retired__ <> %s"%(
+        sql = "select id from _%s where _%s=%s and __retired__=%s"%(
             self.classname, self.key, self.db.arg, self.db.arg)
-        self.db.sql(sql, (keyvalue, 1))
+        self.db.sql(sql, (keyvalue, 0))
 
         # see if there was a result that's not retired
         row = self.db.sql_fetchone()
@@ -1947,8 +1985,8 @@ def find(self, **propspec):
                 s += '_%s in (%s)'%(prop, ','.join([a]*len(values)))
                 where.append('(' + s +')')
         if where:
-            allvalues = (1, ) + allvalues
-            sql.append("""select id from _%s where  __retired__ <> %s
+            allvalues = (0, ) + allvalues
+            sql.append("""select id from _%s where  __retired__=%s
                 and %s"""%(self.classname, a, ' and '.join(where)))
 
         # now multilinks
@@ -1957,15 +1995,15 @@ def find(self, **propspec):
                 continue
             if not values:
                 continue
-            allvalues += (1, )
+            allvalues += (0, )
             if type(values) is type(''):
                 allvalues += (values,)
                 s = a
             else:
                 allvalues += tuple(values.keys())
                 s = ','.join([a]*len(values))
             tn = '%s_%s'%(self.classname, prop)
-            sql.append("""select id from _%s, %s where  __retired__ <> %s
+            sql.append("""select id from _%s, %s where  __retired__=%s
                   and id = %s.nodeid and %s.linkid in (%s)"""%(self.classname,
                   tn, a, tn, tn, s))
 
@@ -1996,9 +2034,9 @@ def stringFind(self, **requirements):
 
         # generate the where clause
         s = ' and '.join(['lower(_%s)=%s'%(col, self.db.arg) for col in where])
-        sql = 'select id from _%s where %s and __retired__<>%s'%(
+        sql = 'select id from _%s where %s and __retired__=%s'%(
             self.classname, s, self.db.arg)
-        args.append(1)
+        args.append(0)
         self.db.sql(sql, tuple(args))
         # XXX numeric ids
         l = [str(x[0]) for x in self.db.sql_fetchall()]
@@ -2017,12 +2055,13 @@ def getnodeids(self, retired=None):
         """
         # flip the sense of the 'retired' flag if we don't want all of them
         if retired is not None:
+            args = (0, )
             if retired:
-                args = (0, )
+                compare = '>'
             else:
-                args = (1, )
-            sql = 'select id from _%s where __retired__ <> %s'%(self.classname,
-                self.db.arg)
+                compare = '='
+            sql = 'select id from _%s where __retired__%s%s'%(self.classname,
+                compare, self.db.arg)
         else:
             args = ()
             sql = 'select id from _%s'%self.classname
@@ -2276,7 +2315,7 @@ def filter(self, search_matches, filterspec, sort=[], group=[]):
         props = self.getprops()
 
         # don't match retired nodes
-        where.append('_%s.__retired__ <> 1'%icn)
+        where.append('_%s.__retired__=0'%icn)
 
         # add results of full text search
         if search_matches is not None:
@@ -2341,7 +2380,7 @@ def filter_sql(self, sql):
         The SQL select must include the item id as the first column.
 
         This function DOES NOT filter out retired items, add on a where
-        clause "__retired__ <> 1" if you don't want retired nodes.
+        clause "__retired__=0" if you don't want retired nodes.
         """
         if __debug__:
             start_t = time.time()
@@ -2502,7 +2541,7 @@ def import_list(self, propnames, proplist):
             # conversion (hello, sqlite)
             sql = 'update _%s set __retired__=%s where id=%s'%(self.classname,
                 self.db.arg, self.db.arg)
-            self.db.sql(sql, (1, newid))
+            self.db.sql(sql, (newid, newid))
         return newid
 
     def export_journals(self):

test/db_test_base.py

@@ -15,7 +15,7 @@
 # BASIS, AND THERE IS NO OBLIGATION WHATSOEVER TO PROVIDE MAINTENANCE,
 # SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.
 #
-# $Id: db_test_base.py,v 1.95 2007-12-23 01:52:07 richard Exp $
+# $Id: db_test_base.py,v 1.96 2008-02-07 03:28:34 richard Exp $
 
 import unittest, os, shutil, errno, imp, sys, time, pprint, sets, base64, os.path
 
@@ -2054,12 +2054,15 @@ def testCreation(self):
 
         # check the basics of the schema and initial data set
         l = db.priority.list()
+        l.sort()
         ae(l, ['1', '2', '3', '4', '5'])
         l = db.status.list()
+        l.sort()
         ae(l, ['1', '2', '3', '4', '5', '6', '7', '8'])
         l = db.keyword.list()
         ae(l, [])
         l = db.user.list()
+        l.sort()
         ae(l, ['1', '2'])
         l = db.msg.list()
         ae(l, [])