[master] Merge branch 'trac324new2' with fixing conflict for

	src/lib/python/isc/notify/tests/testdata/test.sqlite3

compatcheck/Makefile.am

@@ -1,8 +1,12 @@
-noinst_SCRIPTS = sqlite3-difftbl-check.py
-
 # We're going to abuse install-data-local for a pre-install check.
 # This is to be considered a short term hack and is expected to be removed
 # in a near future version.
 install-data-local:
-	$(PYTHON) sqlite3-difftbl-check.py \
-	$(localstatedir)/$(PACKAGE)/zone.sqlite3
+	if test -e $(localstatedir)/$(PACKAGE)/zone.sqlite3; then \
+		$(SHELL) $(top_builddir)/src/bin/dbutil/run_dbutil.sh --check \
+		$(localstatedir)/$(PACKAGE)/zone.sqlite3 || \
+		(echo "\nSQLite3 DB file schema version is old.  " \
+		"Please run: " \
+		"$(abs_top_builddir)/src/bin/dbutil/run_dbutil.sh --upgrade " \
+		"$(localstatedir)/$(PACKAGE)/zone.sqlite3";  exit 1) \
+	fi

compatcheck/sqlite3-difftbl-check.py.in

@@ -1,60 +0,0 @@
-#!@PYTHON@
-
-# Copyright (C) 2011  Internet Systems Consortium.
-#
-# Permission to use, copy, modify, and distribute this software for any
-# purpose with or without fee is hereby granted, provided that the above
-# copyright notice and this permission notice appear in all copies.
-#
-# THE SOFTWARE IS PROVIDED "AS IS" AND INTERNET SYSTEMS CONSORTIUM
-# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
-# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
-# INTERNET SYSTEMS CONSORTIUM BE LIABLE FOR ANY SPECIAL, DIRECT,
-# INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
-# FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
-# NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
-# WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-
-import os, sqlite3, sys
-from optparse import OptionParser
-
-usage = 'usage: %prog [options] db_file'
-parser = OptionParser(usage=usage)
-parser.add_option("-u", "--upgrade", action="store_true",
-                  dest="upgrade", default=False,
-                  help="Upgrade the database file [default: %default]")
-(options, args) = parser.parse_args()
-if len(args) == 0:
-    parser.error('missing argument')
-
-db_file = args[0]
-
-# If the file doesn't exist, there's nothing to do
-if not os.path.exists(db_file):
-    sys.exit(0)
-
-conn = sqlite3.connect(db_file)
-cur = conn.cursor()
-try:
-    # This can be anything that works iff the "diffs" table exists
-    cur.execute('SELECT name FROM diffs DESC LIMIT 1')
-except sqlite3.OperationalError as ex:
-    # If it fails with 'no such table', create a new one or fail with
-    # warning depending on the --upgrade command line option.
-    if str(ex) == 'no such table: diffs':
-        if options.upgrade:
-            cur.execute('CREATE TABLE diffs (id INTEGER PRIMARY KEY, ' +
-                        'zone_id INTEGER NOT NULL, ' +
-                        'version INTEGER NOT NULL, ' +
-                        'operation INTEGER NOT NULL, ' +
-                        'name STRING NOT NULL COLLATE NOCASE, ' +
-                        'rrtype STRING NOT NULL COLLATE NOCASE, ' +
-                        'ttl INTEGER NOT NULL, rdata STRING NOT NULL)')
-        else:
-            sys.stdout.write('Found an older version of SQLite3 DB file: ' +
-                             db_file + '\n' + "Perform '" + os.getcwd() +
-                             "/sqlite3-difftbl-check.py --upgrade " +
-                             db_file + "'\n" +
-                             'before continuing install.\n')
-            sys.exit(1)
-conn.close()

configure.ac

@@ -1139,7 +1139,6 @@ AC_CONFIG_FILES([Makefile
                  tests/tools/perfdhcp/Makefile
                ])
 AC_OUTPUT([doc/version.ent
-           compatcheck/sqlite3-difftbl-check.py
            src/bin/cfgmgr/b10-cfgmgr.py
            src/bin/cfgmgr/tests/b10-cfgmgr_test.py
            src/bin/cmdctl/cmdctl.py

src/bin/dbutil/dbutil.py.in

@@ -378,10 +378,7 @@ def get_latest_version():
 
     This is the 'to' version held in the last element of the upgrades list
     """
-    # Temporarily hardcoded to return 1 as the schema version, until
-    # #324 is merged.
-    #return UPGRADES[-1]['to']
-    return (1, 0)
+    return UPGRADES[-1]['to']
 
 
 def get_version(db):

src/bin/dbutil/tests/dbutil_test.sh.in

@@ -359,22 +359,19 @@ check_version $testdata/old_v1.sqlite3 "V1.0"
 check_no_backup $tempfile $backupfile
 rm -f $tempfile $backupfile
 
-# Temporarily disabled until #324 is merged
-#echo "5.2. Database is an old V1 database - upgrade"
-#upgrade_ok_test $testdata/old_v1.sqlite3 $backupfile
-#rm -f $tempfile $backupfile
+echo "5.2. Database is an old V1 database - upgrade"
+upgrade_ok_test $testdata/old_v1.sqlite3 $backupfile
+rm -f $tempfile $backupfile
 
 
-# Temporarily disabled until #324 is merged
-#echo "6.1. Database is new V1 database - check"
-#check_version $testdata/new_v1.sqlite3 "V1.0"
-#check_no_backup $tempfile $backupfile
-#rm -f $tempfile $backupfile
+echo "6.1. Database is new V1 database - check"
+check_version $testdata/new_v1.sqlite3 "V1.0"
+check_no_backup $tempfile $backupfile
+rm -f $tempfile $backupfile
 
-# Temporarily disabled until #324 is merged
-#echo "6.2. Database is a new V1 database - upgrade"
-#upgrade_ok_test $testdata/new_v1.sqlite3 $backupfile
-#rm -f $tempfile $backupfile
+echo "6.2. Database is a new V1 database - upgrade"
+upgrade_ok_test $testdata/new_v1.sqlite3 $backupfile
+rm -f $tempfile $backupfile
 
 
 echo "7.1. Database is V2.0 database - check"
@@ -405,10 +402,9 @@ upgrade_fail_test $testdata/too_many_version.sqlite3 $backupfile
 rm -f $tempfile $backupfile
 
 
-# Temporarily disabled until #324 is merged
-#echo "10.0. Upgrade corrupt database"
-#upgrade_fail_test $testdata/corrupt.sqlite3 $backupfile
-#rm -f $tempfile $backupfile
+echo "10.0. Upgrade corrupt database"
+upgrade_fail_test $testdata/corrupt.sqlite3 $backupfile
+rm -f $tempfile $backupfile
 
 
 echo "11. Record count test"
@@ -447,15 +443,14 @@ copy_file $testdata/old_v1.sqlite3 $tempfile
 passzero $?
 rm -f $tempfile $backupfile
 
-# Temporarily disabled until #324 is merged
-#echo "13.3 Interactive prompt - yes"
-#copy_file $testdata/old_v1.sqlite3 $tempfile
-#../run_dbutil.sh --upgrade $tempfile << .
-#Yes
-#.
-#passzero $?
-#check_version $tempfile "V2.0"
-#rm -f $tempfile $backupfile
+echo "13.3 Interactive prompt - yes"
+copy_file $testdata/old_v1.sqlite3 $tempfile
+../run_dbutil.sh --upgrade $tempfile << .
+Yes
+.
+passzero $?
+check_version $tempfile "V2.0"
+rm -f $tempfile $backupfile
 
 echo "13.4 Interactive prompt - no"
 copy_file $testdata/old_v1.sqlite3 $tempfile

src/bin/zonemgr/tests/Makefile.am

@@ -20,6 +20,7 @@ endif
 	for pytest in $(PYTESTS) ; do \
 	echo Running test: $$pytest ; \
 	$(LIBRARY_PATH_PLACEHOLDER) \
+	TESTDATAOBJDIR=$(abs_top_builddir)/src/bin/zonemgr/tests/ \
 	B10_FROM_BUILD=$(abs_top_builddir) \
 	PYTHONPATH=$(COMMON_PYTHON_PATH):$(abs_top_builddir)/src/bin/zonemgr:$(abs_top_builddir)/src/lib/dns/python/.libs:$(abs_top_builddir)/src/lib/xfr/.libs \
 	$(PYCOVERAGE_RUN) $(abs_srcdir)/$$pytest || exit ; \

src/bin/zonemgr/tests/zonemgr_test.py

@@ -35,6 +35,8 @@ LOWERBOUND_RETRY = 5
 REFRESH_JITTER = 0.10
 RELOAD_JITTER = 0.75
 
+TEST_SQLITE3_DBFILE = os.getenv("TESTDATAOBJDIR") + '/initdb.file'
+
 class ZonemgrTestException(Exception):
     pass
 
@@ -57,7 +59,7 @@ class FakeCCSession(isc.config.ConfigData, MockModuleCCSession):
 
     def get_remote_config_value(self, module_name, identifier):
         if module_name == "Auth" and identifier == "database_file":
-            return "initdb.file", False
+            return TEST_SQLITE3_DBFILE, False
         else:
             return "unknown", False
 
@@ -81,7 +83,7 @@ class MyZonemgrRefresh(ZonemgrRefresh):
                 return None
         sqlite3_ds.get_zone_soa = get_zone_soa
 
-        ZonemgrRefresh.__init__(self, MySession(), "initdb.file",
+        ZonemgrRefresh.__init__(self, MySession(), TEST_SQLITE3_DBFILE,
                                 self._slave_socket, FakeCCSession())
         current_time = time.time()
         self._zonemgr_refresh_info = {
@@ -99,11 +101,18 @@ class MyZonemgrRefresh(ZonemgrRefresh):
 
 class TestZonemgrRefresh(unittest.TestCase):
     def setUp(self):
+        if os.path.exists(TEST_SQLITE3_DBFILE):
+            os.unlink(TEST_SQLITE3_DBFILE)
         self.stderr_backup = sys.stderr
         sys.stderr = open(os.devnull, 'w')
         self.zone_refresh = MyZonemgrRefresh()
         self.cc_session = FakeCCSession()
 
+    def tearDown(self):
+        if os.path.exists(TEST_SQLITE3_DBFILE):
+            os.unlink(TEST_SQLITE3_DBFILE)
+        sys.stderr = self.stderr_backup
+
     def test_random_jitter(self):
         max = 100025.120
         jitter = 0
@@ -602,13 +611,10 @@ class TestZonemgrRefresh(unittest.TestCase):
                           self.zone_refresh.update_config_data,
                           config, self.cc_session)
 
-    def tearDown(self):
-        sys.stderr= self.stderr_backup
-
 class MyZonemgr(Zonemgr):
 
     def __init__(self):
-        self._db_file = "initdb.file"
+        self._db_file = TEST_SQLITE3_DBFILE
         self._zone_refresh = None
         self._shutdown_event = threading.Event()
         self._cc = MySession()
@@ -628,8 +634,14 @@ class MyZonemgr(Zonemgr):
 class TestZonemgr(unittest.TestCase):
 
     def setUp(self):
+        if os.path.exists(TEST_SQLITE3_DBFILE):
+            os.unlink(TEST_SQLITE3_DBFILE)
         self.zonemgr = MyZonemgr()
 
+    def tearDown(self):
+        if os.path.exists(TEST_SQLITE3_DBFILE):
+            os.unlink(TEST_SQLITE3_DBFILE)
+
     def test_config_handler(self):
         config_data1 = {
                     "lowerbound_refresh" : 60,
@@ -650,8 +662,8 @@ class TestZonemgr(unittest.TestCase):
         self.zonemgr.config_handler(config_data3)
         self.assertEqual(0.5, self.zonemgr._config_data.get("refresh_jitter"))
         # The zone doesn't exist in database, simply skip loading soa for it and log an warning
-        self.zonemgr._zone_refresh = ZonemgrRefresh(None, "initdb.file", None,
-                                                    FakeCCSession())
+        self.zonemgr._zone_refresh = ZonemgrRefresh(None, TEST_SQLITE3_DBFILE,
+                                                    None, FakeCCSession())
         config_data1["secondary_zones"] = [{"name": "nonexistent.example",
                                             "class": "IN"}]
         self.assertEqual(self.zonemgr.config_handler(config_data1),
@@ -663,7 +675,7 @@ class TestZonemgr(unittest.TestCase):
         self.assertEqual(0.1, self.zonemgr._config_data.get("refresh_jitter"))
 
     def test_get_db_file(self):
-        self.assertEqual("initdb.file", self.zonemgr.get_db_file())
+        self.assertEqual(TEST_SQLITE3_DBFILE, self.zonemgr.get_db_file())
 
     def test_parse_cmd_params(self):
         params1 = {"zone_name" : "example.com.", "zone_class" : "CH", "master" : "127.0.0.1"}
@@ -691,9 +703,6 @@ class TestZonemgr(unittest.TestCase):
         self.zonemgr.run()
         self.assertTrue(self.zonemgr._module_cc.stopped)
 
-    def tearDown(self):
-        pass
-
 if __name__== "__main__":
     isc.log.resetUnitTestRootLogger()
     unittest.main()

src/lib/datasrc/datasrc_messages.mes

@@ -634,6 +634,17 @@ enough information for it.  The code is 1 for error, 2 for not implemented.
 % DATASRC_SQLITE_CLOSE closing SQLite database
 Debug information. The SQLite data source is closing the database file.
 
+% DATASRC_SQLITE_COMPATIBLE_VERSION database schema V%1.%2 not up to date (expecting V%3.%4) but is compatible
+The version of the SQLite3 database schema used to hold the zone data
+is not the latest one - the current version of BIND 10 was written
+with a later schema version in mind.  However, the database is
+compatible with the current version of BIND 10, and BIND 10 will run
+without any problems.
+
+Consult the release notes for your version of BIND 10.  Depending on
+the changes made to the database schema, it is possible that improved
+performance could result if the database were upgraded.
+
 % DATASRC_SQLITE_CONNCLOSE Closing sqlite database
 The database file is no longer needed and is being closed.
 
@@ -701,6 +712,14 @@ source.
 The SQLite data source was asked to provide a NSEC3 record for given zone.
 But it doesn't contain that zone.
 
+% DATASRC_SQLITE_INCOMPATIBLE_VERSION database schema V%1.%2 incompatible with version (V%3.%4) expected
+The version of the SQLite3 database schema used to hold the zone data
+is incompatible with the version expected by BIND 10.  As a result,
+BIND 10 is unable to run using the database file as the data source.
+
+The database should be updated using the means described in the BIND
+10 documentation.
+
 % DATASRC_SQLITE_NEWCONN SQLite3Database is being initialized
 A wrapper object to hold database connection is being initialized.
 

src/lib/datasrc/sqlite3_accessor.cc

@@ -15,8 +15,11 @@
 #include <sqlite3.h>
 
 #include <string>
+#include <utility>
 #include <vector>
 
+#include <exceptions/exceptions.h>
+
 #include <datasrc/sqlite3_accessor.h>
 #include <datasrc/logger.h>
 #include <datasrc/data_source.h>
@@ -27,7 +30,20 @@
 using namespace std;
 using namespace isc::data;
 
-#define SQLITE_SCHEMA_VERSION 1
+namespace {
+// Expected schema.  The major version must match else there is an error.  If
+// the minor version of the database is less than this, a warning is output.
+//
+// It is assumed that a program written to run on m.n of the database will run
+// with a database version m.p, where p is any number.  However, if p < n,
+// we assume that the database structure was upgraded for some reason, and that
+// some advantage may result if the database is upgraded. Conversely, if p > n,
+// The database is at a later version than the program was written for and the
+// program may not be taking advantage of features (possibly performance
+// improvements) added to the database.
+const int SQLITE_SCHEMA_MAJOR_VERSION = 2;
+const int SQLITE_SCHEMA_MINOR_VERSION = 0;
+}
 
 namespace isc {
 namespace datasrc {
@@ -125,8 +141,8 @@ const char* const text_statements[NUM_STATEMENTS] = {
 
 struct SQLite3Parameters {
     SQLite3Parameters() :
-        db_(NULL), version_(-1), in_transaction(false), updating_zone(false),
-        updated_zone_id(-1)
+        db_(NULL), major_version_(-1), minor_version_(-1),
+        in_transaction(false), updating_zone(false), updated_zone_id(-1)
     {
         for (int i = 0; i < NUM_STATEMENTS; ++i) {
             statements_[i] = NULL;
@@ -164,7 +180,8 @@ struct SQLite3Parameters {
     }
 
     sqlite3* db_;
-    int version_;
+    int major_version_;
+    int minor_version_;
     bool in_transaction; // whether or not a transaction has been started
     bool updating_zone;          // whether or not updating the zone
     int updated_zone_id;        // valid only when in_transaction is true
@@ -255,34 +272,42 @@ public:
 };
 
 const char* const SCHEMA_LIST[] = {
-    "CREATE TABLE schema_version (version INTEGER NOT NULL)",
-    "INSERT INTO schema_version VALUES (1)",
+    "CREATE TABLE schema_version (version INTEGER NOT NULL, "
+        "minor INTEGER NOT NULL DEFAULT 0)",
+    "INSERT INTO schema_version VALUES (2, 0)",
     "CREATE TABLE zones (id INTEGER PRIMARY KEY, "
-    "name STRING NOT NULL COLLATE NOCASE, "
-    "rdclass STRING NOT NULL COLLATE NOCASE DEFAULT 'IN', "
+    "name TEXT NOT NULL COLLATE NOCASE, "
+    "rdclass TEXT NOT NULL COLLATE NOCASE DEFAULT 'IN', "
     "dnssec BOOLEAN NOT NULL DEFAULT 0)",
     "CREATE INDEX zones_byname ON zones (name)",
     "CREATE TABLE records (id INTEGER PRIMARY KEY, "
-        "zone_id INTEGER NOT NULL, name STRING NOT NULL COLLATE NOCASE, "
-        "rname STRING NOT NULL COLLATE NOCASE, ttl INTEGER NOT NULL, "
-        "rdtype STRING NOT NULL COLLATE NOCASE, sigtype STRING COLLATE NOCASE, "
-        "rdata STRING NOT NULL)",
+        "zone_id INTEGER NOT NULL, name TEXT NOT NULL COLLATE NOCASE, "
+        "rname TEXT NOT NULL COLLATE NOCASE, ttl INTEGER NOT NULL, "
+        "rdtype TEXT NOT NULL COLLATE NOCASE, sigtype TEXT COLLATE NOCASE, "
+        "rdata TEXT NOT NULL)",
     "CREATE INDEX records_byname ON records (name)",
     "CREATE INDEX records_byrname ON records (rname)",
+    // The next index is a tricky one.  It's necessary for
+    // FIND_PREVIOUS to use the index efficiently; since there's an
+    // "inequality", the rname column must be placed later.  records_byrname
+    // may not be sufficient especially when the zone is not signed (and
+    // defining a separate index for rdtype only doesn't work either; SQLite3
+    // would then create a temporary B-tree for "ORDER BY").
+    "CREATE INDEX records_bytype_and_rname ON records (rdtype, rname)",
     "CREATE TABLE nsec3 (id INTEGER PRIMARY KEY, zone_id INTEGER NOT NULL, "
-        "hash STRING NOT NULL COLLATE NOCASE, "
-        "owner STRING NOT NULL COLLATE NOCASE, "
-        "ttl INTEGER NOT NULL, rdtype STRING NOT NULL COLLATE NOCASE, "
-        "rdata STRING NOT NULL)",
+        "hash TEXT NOT NULL COLLATE NOCASE, "
+        "owner TEXT NOT NULL COLLATE NOCASE, "
+        "ttl INTEGER NOT NULL, rdtype TEXT NOT NULL COLLATE NOCASE, "
+        "rdata TEXT NOT NULL)",
     "CREATE INDEX nsec3_byhash ON nsec3 (hash)",
     "CREATE TABLE diffs (id INTEGER PRIMARY KEY, "
         "zone_id INTEGER NOT NULL, "
         "version INTEGER NOT NULL, "
         "operation INTEGER NOT NULL, "
-        "name STRING NOT NULL COLLATE NOCASE, "
-        "rrtype STRING NOT NULL COLLATE NOCASE, "
+        "name TEXT NOT NULL COLLATE NOCASE, "
+        "rrtype TEXT NOT NULL COLLATE NOCASE, "
         "ttl INTEGER NOT NULL, "
-        "rdata STRING NOT NULL)",
+        "rdata TEXT NOT NULL)",
     NULL
 };
 
@@ -308,14 +333,13 @@ void doSleep() {
 
 // returns the schema version if the schema version table exists
 // returns -1 if it does not
-int checkSchemaVersion(sqlite3* db) {
+int checkSchemaVersionElement(sqlite3* db, const char* const query) {
     sqlite3_stmt* prepared = NULL;
     // At this point in time, the database might be exclusively locked, in
     // which case even prepare() will return BUSY, so we may need to try a
     // few times
     for (size_t i = 0; i < 50; ++i) {
-        int rc = sqlite3_prepare_v2(db, "SELECT version FROM schema_version",
-                                    -1, &prepared, NULL);
+        int rc = sqlite3_prepare_v2(db, query, -1, &prepared, NULL);
         if (rc == SQLITE_ERROR) {
             // this is the error that is returned when the table does not
             // exist
@@ -337,50 +361,116 @@ int checkSchemaVersion(sqlite3* db) {
     return (version);
 }
 
+// Returns the schema major and minor version numbers in a pair.
+// Returns (-1, -1) if the table does not exist, (1, 0) for a V1
+// database, and (n, m) for any other.
+pair<int, int> checkSchemaVersion(sqlite3* db) {
+    int major = checkSchemaVersionElement(db,
+        "SELECT version FROM schema_version");
+    if (major == -1) {
+        return (make_pair(-1, -1));
+    } else if (major == 1) {
+        return (make_pair(1, 0));
+    } else {
+        int minor = checkSchemaVersionElement(db,
+            "SELECT minor FROM schema_version");
+        return (make_pair(major, minor));
+    }
+}
+
+// A helper class used in createDatabase() below so we manage the one shot
+// transaction safely.
+class ScopedTransaction {
+public:
+    ScopedTransaction(sqlite3* db) : db_(NULL) {
+        // try for 5 secs (50*0.1)
+        for (size_t i = 0; i < 50; ++i) {
+            const int rc = sqlite3_exec(db, "BEGIN EXCLUSIVE TRANSACTION",
+                                        NULL, NULL, NULL);
+            if (rc == SQLITE_OK) {
+                break;
+            } else if (rc != SQLITE_BUSY || i == 50) {
+                isc_throw(SQLite3Error, "Unable to acquire exclusive lock "
+                          "for database creation: " << sqlite3_errmsg(db));
+            }
+            doSleep();
+        }
+        // Hold the DB pointer once we have successfully acquired the lock.
+        db_ = db;
+    }
+    ~ScopedTransaction() {
+        if (db_ != NULL) {
+            // Note: even rollback could fail in theory, but in that case
+            // we cannot do much for safe recovery anyway.  We could at least
+            // log the event, but for now don't even bother to do that, with
+            // the expectation that we'll soon stop creating the schema in this
+            // module.
+            sqlite3_exec(db_, "ROLLBACK", NULL, NULL, NULL);
+        }
+    }
+    void commit() {
+        if (sqlite3_exec(db_, "COMMIT TRANSACTION", NULL, NULL, NULL) !=
+            SQLITE_OK) {
+            isc_throw(SQLite3Error, "Unable to commit newly created database "
+                      "schema: " << sqlite3_errmsg(db_));
+        }
+        db_ = NULL;
+    }
+
+private:
+    sqlite3* db_;
+};
+
 // return db version
-int create_database(sqlite3* db) {
+pair<int, int>
+createDatabase(sqlite3* db) {
+    logger.info(DATASRC_SQLITE_SETUP);
+
     // try to get an exclusive lock. Once that is obtained, do the version
     // check *again*, just in case this process was racing another
-    //
-    // try for 5 secs (50*0.1)
-    int rc;
-    logger.info(DATASRC_SQLITE_SETUP);
-    for (size_t i = 0; i < 50; ++i) {
-        rc = sqlite3_exec(db, "BEGIN EXCLUSIVE TRANSACTION", NULL, NULL,
-                            NULL);
-        if (rc == SQLITE_OK) {
-            break;
-        } else if (rc != SQLITE_BUSY || i == 50) {
-            isc_throw(SQLite3Error, "Unable to acquire exclusive lock "
-                        "for database creation: " << sqlite3_errmsg(db));
-        }
-        doSleep();
-    }
-    int schema_version = checkSchemaVersion(db);
-    if (schema_version == -1) {
+    ScopedTransaction trasaction(db);
+    pair<int, int> schema_version = checkSchemaVersion(db);
+    if (schema_version.first == -1) {
         for (int i = 0; SCHEMA_LIST[i] != NULL; ++i) {
             if (sqlite3_exec(db, SCHEMA_LIST[i], NULL, NULL, NULL) !=
                 SQLITE_OK) {
                 isc_throw(SQLite3Error,
-                        "Failed to set up schema " << SCHEMA_LIST[i]);
+                          "Failed to set up schema " << SCHEMA_LIST[i]);
             }
         }
-        sqlite3_exec(db, "COMMIT TRANSACTION", NULL, NULL, NULL);
-        return (SQLITE_SCHEMA_VERSION);
-    } else {
-        return (schema_version);
+        trasaction.commit();
+
+        // Return the version.  We query again to ensure that the only point
+        // in which the current schema version is defined is in the create
+        // statements.
+        schema_version = checkSchemaVersion(db);
     }
+
+    return (schema_version);
 }
 
 void
 checkAndSetupSchema(Initializer* initializer) {
     sqlite3* const db = initializer->params_.db_;
 
-    int schema_version = checkSchemaVersion(db);
-    if (schema_version != SQLITE_SCHEMA_VERSION) {
-        schema_version = create_database(db);
-    }
-    initializer->params_.version_ = schema_version;
+    pair<int, int> schema_version = checkSchemaVersion(db);
+    if (schema_version.first == -1) {
+        schema_version = createDatabase(db);
+    } else if (schema_version.first != SQLITE_SCHEMA_MAJOR_VERSION) {
+        LOG_ERROR(logger, DATASRC_SQLITE_INCOMPATIBLE_VERSION)
+            .arg(schema_version.first).arg(schema_version.second)
+            .arg(SQLITE_SCHEMA_MAJOR_VERSION).arg(SQLITE_SCHEMA_MINOR_VERSION);
+        isc_throw(IncompatibleDbVersion,
+                  "incompatible SQLite3 database version: " <<
+                  schema_version.first << "." << schema_version.second);
+    } else if (schema_version.second < SQLITE_SCHEMA_MINOR_VERSION) {
+        LOG_WARN(logger, DATASRC_SQLITE_COMPATIBLE_VERSION)
+            .arg(schema_version.first).arg(schema_version.second)
+            .arg(SQLITE_SCHEMA_MAJOR_VERSION).arg(SQLITE_SCHEMA_MINOR_VERSION);
+    }
+
+    initializer->params_.major_version_ = schema_version.first;
+    initializer->params_.minor_version_ = schema_version.second;
 }
 
 }

src/lib/datasrc/sqlite3_accessor.h

@@ -47,6 +47,12 @@ public:
         DataSourceError(file, line, what) {}
 };
 
+class IncompatibleDbVersion : public Exception {
+public:
+    IncompatibleDbVersion(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what) {}
+};
+
 /**
  * \brief Too Much Data
  *

src/lib/datasrc/sqlite3_datasrc.cc

@@ -14,19 +14,33 @@
 
 #include <string>
 #include <sstream>
+#include <utility>
 
 #include <sqlite3.h>
 
 #include <datasrc/sqlite3_datasrc.h>
 #include <datasrc/logger.h>
-
+#include <exceptions/exceptions.h>
 #include <dns/rrttl.h>
 #include <dns/rdata.h>
 #include <dns/rdataclass.h>
 #include <dns/rrset.h>
 #include <dns/rrsetlist.h>
 
-#define SQLITE_SCHEMA_VERSION 1
+namespace {
+// Expected schema.  The major version must match else there is an error.  If
+// the minor version of the database is less than this, a warning is output.
+//
+// It is assumed that a program written to run on m.n of the database will run
+// with a database version m.p, where p is any number.  However, if p < n,
+// we assume that the database structure was upgraded for some reason, and that
+// some advantage may result if the database is upgraded. Conversely, if p > n,
+// The database is at a later version than the program was written for and the
+// program may not be taking advantage of features (possibly performance
+// improvements) added to the database.
+const int SQLITE_SCHEMA_MAJOR_VERSION = 2;
+const int SQLITE_SCHEMA_MINOR_VERSION = 0;
+}
 
 using namespace std;
 using namespace isc::dns;
@@ -36,13 +50,14 @@ namespace isc {
 namespace datasrc {
 
 struct Sqlite3Parameters {
-    Sqlite3Parameters() :  db_(NULL), version_(-1),
+    Sqlite3Parameters() :  db_(NULL), major_version_(-1), minor_version_(-1),
         q_zone_(NULL), q_record_(NULL), q_addrs_(NULL), q_referral_(NULL),
         q_any_(NULL), q_count_(NULL), q_previous_(NULL), q_nsec3_(NULL),
         q_prevnsec3_(NULL)
     {}
     sqlite3* db_;
-    int version_;
+    int major_version_;
+    int minor_version_;
     sqlite3_stmt* q_zone_;
     sqlite3_stmt* q_record_;
     sqlite3_stmt* q_addrs_;
@@ -56,38 +71,41 @@ struct Sqlite3Parameters {
 
 namespace {
 const char* const SCHEMA_LIST[] = {
-    "CREATE TABLE schema_version (version INTEGER NOT NULL)",
-    "INSERT INTO schema_version VALUES (1)",
+    "CREATE TABLE schema_version (version INTEGER NOT NULL, "
+        "minor INTEGER NOT NULL DEFAULT 0)",
+    "INSERT INTO schema_version VALUES (2, 0)",
     "CREATE TABLE zones (id INTEGER PRIMARY KEY, "
-    "name STRING NOT NULL COLLATE NOCASE, "
-    "rdclass STRING NOT NULL COLLATE NOCASE DEFAULT 'IN', "
+    "name TEXT NOT NULL COLLATE NOCASE, "
+    "rdclass TEXT NOT NULL COLLATE NOCASE DEFAULT 'IN', "
     "dnssec BOOLEAN NOT NULL DEFAULT 0)",
     "CREATE INDEX zones_byname ON zones (name)",
     "CREATE TABLE records (id INTEGER PRIMARY KEY, "
-    "zone_id INTEGER NOT NULL, name STRING NOT NULL COLLATE NOCASE, "
-    "rname STRING NOT NULL COLLATE NOCASE, ttl INTEGER NOT NULL, "
-    "rdtype STRING NOT NULL COLLATE NOCASE, sigtype STRING COLLATE NOCASE, "
-    "rdata STRING NOT NULL)",
+    "zone_id INTEGER NOT NULL, name TEXT NOT NULL COLLATE NOCASE, "
+    "rname TEXT NOT NULL COLLATE NOCASE, ttl INTEGER NOT NULL, "
+    "rdtype TEXT NOT NULL COLLATE NOCASE, sigtype TEXT COLLATE NOCASE, "
+    "rdata TEXT NOT NULL)",
     "CREATE INDEX records_byname ON records (name)",
     "CREATE INDEX records_byrname ON records (rname)",
+    "CREATE INDEX records_bytype_and_rname ON records (rdtype, rname)",
     "CREATE TABLE nsec3 (id INTEGER PRIMARY KEY, zone_id INTEGER NOT NULL, "
-    "hash STRING NOT NULL COLLATE NOCASE, "
-    "owner STRING NOT NULL COLLATE NOCASE, "
-    "ttl INTEGER NOT NULL, rdtype STRING NOT NULL COLLATE NOCASE, "
-    "rdata STRING NOT NULL)",
+    "hash TEXT NOT NULL COLLATE NOCASE, "
+    "owner TEXT NOT NULL COLLATE NOCASE, "
+    "ttl INTEGER NOT NULL, rdtype TEXT NOT NULL COLLATE NOCASE, "
+    "rdata TEXT NOT NULL)",
     "CREATE INDEX nsec3_byhash ON nsec3 (hash)",
     "CREATE TABLE diffs (id INTEGER PRIMARY KEY, "
         "zone_id INTEGER NOT NULL, "
         "version INTEGER NOT NULL, "
         "operation INTEGER NOT NULL, "
-        "name STRING NOT NULL COLLATE NOCASE, "
-        "rrtype STRING NOT NULL COLLATE NOCASE, "
+        "name TEXT NOT NULL COLLATE NOCASE, "
+        "rrtype TEXT NOT NULL COLLATE NOCASE, "
         "ttl INTEGER NOT NULL, "
-        "rdata STRING NOT NULL)",
+        "rdata TEXT NOT NULL)",
     NULL
 };
 
 const char* const q_version_str = "SELECT version FROM schema_version";
+const char* const q_minor_str = "SELECT minor FROM schema_version";
 
 const char* const q_zone_str = "SELECT id FROM zones WHERE name=?1";
 
@@ -109,12 +127,16 @@ const char* const q_referral_str = "SELECT rdtype, ttl, sigtype, rdata FROM "
 const char* const q_any_str = "SELECT rdtype, ttl, sigtype, rdata "
     "FROM records WHERE zone_id=?1 AND name=?2";
 
+// Note: the wildcard symbol '%' is expected to be added to the text
+// for the placeholder for LIKE given via sqlite3_bind_text().  We don't
+// use the expression such as (?2 || '%') because it would disable the use
+// of indices and could result in terrible performance.
 const char* const q_count_str = "SELECT COUNT(*) FROM records "
-    "WHERE zone_id=?1 AND rname LIKE (?2 || '%');";
+    "WHERE zone_id=?1 AND rname LIKE ?2;";
 
 const char* const q_previous_str = "SELECT name FROM records "
-    "WHERE zone_id=?1 AND rdtype = 'NSEC' AND "
-    "rname < $2 ORDER BY rname DESC LIMIT 1";
+    "WHERE rname < ?2 AND zone_id=?1 AND rdtype = 'NSEC' "
+    "ORDER BY rname DESC LIMIT 1";
 
 const char* const q_nsec3_str = "SELECT rdtype, ttl, rdata FROM nsec3 "
     "WHERE zone_id = ?1 AND hash = $2";
@@ -314,8 +336,9 @@ Sqlite3DataSrc::findRecords(const Name& name, const RRType& rdtype,
                   " to SQL statement (qcount)");
     }
 
-    const string revname_text = name.reverse().toText();
-    rc = sqlite3_bind_text(dbparameters->q_count_, 2, revname_text.c_str(),
+    const string revname_text = name.reverse().toText() + "%";
+    rc = sqlite3_bind_text(dbparameters->q_count_, 2,
+                           revname_text.c_str(),
                            -1, SQLITE_STATIC);
     if (rc != SQLITE_OK) {
         isc_throw(Sqlite3Error, "Could not bind name " << name.reverse() <<
@@ -675,15 +698,15 @@ void do_sleep() {
     nanosleep(&req, NULL);
 }
 
-// returns the schema version if the schema version table exists
+// returns the schema version element if the schema version table exists
 // returns -1 if it does not
-int check_schema_version(sqlite3* db) {
+int check_schema_version_element(sqlite3* db, const char* const version_query) {
     sqlite3_stmt* prepared = NULL;
     // At this point in time, the database might be exclusively locked, in
     // which case even prepare() will return BUSY, so we may need to try a
     // few times
     for (size_t i = 0; i < 50; ++i) {
-        int rc = sqlite3_prepare_v2(db, q_version_str, -1, &prepared, NULL);
+        int rc = sqlite3_prepare_v2(db, version_query, -1, &prepared, NULL);
         if (rc == SQLITE_ERROR) {
             // this is the error that is returned when the table does not
             // exist
@@ -705,27 +728,73 @@ int check_schema_version(sqlite3* db) {
     return (version);
 }
 
+// Returns the schema major and minor version numbers in a pair.
+// Returns (-1, -1) if the table does not exist, (1, 0) for a V1
+// database, and (n, m) for any other.
+pair<int, int> check_schema_version(sqlite3* db) {
+    int major = check_schema_version_element(db, q_version_str);
+    if (major == -1) {
+        return (make_pair(-1, -1));
+    } else if (major == 1) {
+        return (make_pair(1, 0));
+    } else {
+        int minor = check_schema_version_element(db, q_minor_str);
+        return (make_pair(major, minor));
+    }
+}
+
+// A helper class used in create_database() below so we manage the one shot
+// transaction safely.
+class ScopedTransaction {
+public:
+    ScopedTransaction(sqlite3* db) : db_(NULL) {
+        // try for 5 secs (50*0.1)
+        for (size_t i = 0; i < 50; ++i) {
+            const int rc = sqlite3_exec(db, "BEGIN EXCLUSIVE TRANSACTION",
+                                        NULL, NULL, NULL);
+            if (rc == SQLITE_OK) {
+                break;
+            } else if (rc != SQLITE_BUSY || i == 50) {
+                isc_throw(Sqlite3Error, "Unable to acquire exclusive lock "
+                          "for database creation: " << sqlite3_errmsg(db));
+            }
+            do_sleep();
+        }
+        // Hold the DB pointer once we have successfully acquired the lock.
+        db_ = db;
+    }
+    ~ScopedTransaction() {
+        if (db_ != NULL) {
+            // Note: even rollback could fail in theory, but in that case
+            // we cannot do much for safe recovery anyway.  We could at least
+            // log the event, but for now don't even bother to do that, with
+            // the expectation that we'll soon stop creating the schema in this
+            // module.
+            sqlite3_exec(db_, "ROLLBACK", NULL, NULL, NULL);
+        }
+    }
+    void commit() {
+        if (sqlite3_exec(db_, "COMMIT TRANSACTION", NULL, NULL, NULL) !=
+            SQLITE_OK) {
+            isc_throw(Sqlite3Error, "Unable to commit newly created database "
+                      "schema: " << sqlite3_errmsg(db_));
+        }
+        db_ = NULL;
+    }
+
+private:
+    sqlite3* db_;
+};
+
 // return db version
-int create_database(sqlite3* db) {
+pair<int, int> create_database(sqlite3* db) {
+    logger.info(DATASRC_SQLITE_SETUP);
+
     // try to get an exclusive lock. Once that is obtained, do the version
     // check *again*, just in case this process was racing another
-    //
-    // try for 5 secs (50*0.1)
-    int rc;
-    logger.info(DATASRC_SQLITE_SETUP);
-    for (size_t i = 0; i < 50; ++i) {
-        rc = sqlite3_exec(db, "BEGIN EXCLUSIVE TRANSACTION", NULL, NULL,
-                            NULL);
-        if (rc == SQLITE_OK) {
-            break;
-        } else if (rc != SQLITE_BUSY || i == 50) {
-            isc_throw(Sqlite3Error, "Unable to acquire exclusive lock "
-                        "for database creation: " << sqlite3_errmsg(db));
-        }
-        do_sleep();
-    }
-    int schema_version = check_schema_version(db);
-    if (schema_version == -1) {
+    ScopedTransaction transaction(db);
+    pair<int, int> schema_version = check_schema_version(db);
+    if (schema_version.first == -1) {
         for (int i = 0; SCHEMA_LIST[i] != NULL; ++i) {
             if (sqlite3_exec(db, SCHEMA_LIST[i], NULL, NULL, NULL) !=
                 SQLITE_OK) {
@@ -733,23 +802,40 @@ int create_database(sqlite3* db) {
                         "Failed to set up schema " << SCHEMA_LIST[i]);
             }
         }
-        sqlite3_exec(db, "COMMIT TRANSACTION", NULL, NULL, NULL);
-        return (SQLITE_SCHEMA_VERSION);
-    } else {
-        return (schema_version);
+        transaction.commit();
+
+        // Return the version. We query again to ensure that the only point
+        // in which the current schema version is defined is in the
+        // CREATE statements.
+        schema_version = check_schema_version(db);
     }
+    return (schema_version);
 }
 
 void
 checkAndSetupSchema(Sqlite3Initializer* initializer) {
     sqlite3* const db = initializer->params_.db_;
 
-    int schema_version = check_schema_version(db);
-    if (schema_version != SQLITE_SCHEMA_VERSION) {
+    // Note: we use the same SCHEMA_xxx_VERSION log IDs here and in
+    // sqlite3_accessor.cc, which is against our policy of ID uniqueness.
+    // The assumption is that this file will soon be deprecated, and we don't
+    // bother to define separate IDs for the short period.
+    pair<int, int> schema_version = check_schema_version(db);
+    if (schema_version.first == -1) {
         schema_version = create_database(db);
-    }
-    initializer->params_.version_ = schema_version;
-
+    } else if (schema_version.first != SQLITE_SCHEMA_MAJOR_VERSION) {
+        LOG_ERROR(logger, DATASRC_SQLITE_INCOMPATIBLE_VERSION)
+            .arg(schema_version.first).arg(schema_version.second)
+            .arg(SQLITE_SCHEMA_MAJOR_VERSION).arg(SQLITE_SCHEMA_MINOR_VERSION);
+        isc_throw(IncompatibleDbVersion, "Incompatible database version");
+    } else if (schema_version.second < SQLITE_SCHEMA_MINOR_VERSION) {
+        LOG_WARN(logger, DATASRC_SQLITE_COMPATIBLE_VERSION)
+            .arg(schema_version.first).arg(schema_version.second)
+            .arg(SQLITE_SCHEMA_MAJOR_VERSION).arg(SQLITE_SCHEMA_MINOR_VERSION);
+    }
+
+    initializer->params_.major_version_ = schema_version.first;
+    initializer->params_.minor_version_ = schema_version.second;
     initializer->params_.q_zone_ = prepare(db, q_zone_str);
     initializer->params_.q_record_ = prepare(db, q_record_str);
     initializer->params_.q_addrs_ = prepare(db, q_addrs_str);

src/lib/datasrc/sqlite3_datasrc.h

@@ -41,6 +41,12 @@ public:
         isc::Exception(file, line, what) {}
 };
 
+class IncompatibleDbVersion : public Exception {
+public:
+    IncompatibleDbVersion(const char* file, size_t line, const char* what) :
+        isc::Exception(file, line, what) {}
+};
+
 class Sqlite3DataSrc : public DataSrc {
     ///
     /// \name Constructors, Assignment Operator and Destructor.

src/lib/datasrc/tests/Makefile.am

@@ -110,4 +110,6 @@ EXTRA_DIST += testdata/sql1.example.com.signed
 EXTRA_DIST += testdata/sql2.example.com.signed
 EXTRA_DIST += testdata/test-root.sqlite3
 EXTRA_DIST += testdata/test.sqlite3
-EXTRA_DIST += testdata/test.sqlite3.nodiffs
+EXTRA_DIST += testdata/new_minor_schema.sqlite3
+EXTRA_DIST += testdata/newschema.sqlite3
+EXTRA_DIST += testdata/oldschema.sqlite3

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -37,15 +37,22 @@ using isc::dns::Name;
 
 namespace {
 // Some test data
-std::string SQLITE_DBFILE_EXAMPLE = TEST_DATA_DIR "/test.sqlite3";
-std::string SQLITE_DBFILE_EXAMPLE2 = TEST_DATA_DIR "/example2.com.sqlite3";
-std::string SQLITE_DBNAME_EXAMPLE2 = "sqlite3_example2.com.sqlite3";
-std::string SQLITE_DBFILE_EXAMPLE_ROOT = TEST_DATA_DIR "/test-root.sqlite3";
-std::string SQLITE_DBNAME_EXAMPLE_ROOT = "sqlite3_test-root.sqlite3";
-std::string SQLITE_DBFILE_BROKENDB = TEST_DATA_DIR "/brokendb.sqlite3";
-std::string SQLITE_DBFILE_MEMORY = ":memory:";
-std::string SQLITE_DBFILE_EXAMPLE_ORG = TEST_DATA_DIR "/example.org.sqlite3";
-std::string SQLITE_DBFILE_DIFFS = TEST_DATA_DIR "/diffs.sqlite3";
+const char* const SQLITE_DBFILE_EXAMPLE = TEST_DATA_DIR "/test.sqlite3";
+const char* const SQLITE_DBFILE_EXAMPLE2 =
+    TEST_DATA_DIR "/example2.com.sqlite3";
+const char* const SQLITE_DBNAME_EXAMPLE2 = "sqlite3_example2.com.sqlite3";
+const char* const SQLITE_DBFILE_EXAMPLE_ROOT =
+    TEST_DATA_DIR "/test-root.sqlite3";
+const char* const SQLITE_DBNAME_EXAMPLE_ROOT = "sqlite3_test-root.sqlite3";
+const char* const SQLITE_DBFILE_BROKENDB = TEST_DATA_DIR "/brokendb.sqlite3";
+const char* const SQLITE_DBFILE_MEMORY = ":memory:";
+const char* const SQLITE_DBFILE_EXAMPLE_ORG =
+    TEST_DATA_DIR "/example.org.sqlite3";
+const char* const SQLITE_DBFILE_DIFFS = TEST_DATA_DIR "/diffs.sqlite3";
+const char* const SQLITE_DBFILE_NEWSCHEMA = TEST_DATA_DIR "/newschema.sqlite3";
+const char* const SQLITE_DBFILE_OLDSCHEMA = TEST_DATA_DIR "/oldschema.sqlite3";
+const char* const SQLITE_DBFILE_NEW_MINOR_SCHEMA =
+    TEST_DATA_DIR "/new_minor_schema.sqlite3";
 
 // The following file must be non existent and must be non"creatable";
 // the sqlite3 library will try to create a new DB file if it doesn't exist,
@@ -74,6 +81,20 @@ TEST(SQLite3Open, brokenDB) {
                  SQLite3Error);
 }
 
+// Different schema versions
+TEST(SQLite3Open, differentSchemaVersions) {
+    // If the major version is different from the current one, it should fail.
+    EXPECT_THROW(SQLite3Accessor(SQLITE_DBFILE_NEWSCHEMA, "IN"),
+                 IncompatibleDbVersion);
+    EXPECT_THROW(SQLite3Accessor(SQLITE_DBFILE_OLDSCHEMA, "IN"),
+                 IncompatibleDbVersion);
+
+    // Difference in the minor version is okay (as of this test written
+    // the current minor version is 0, so we can only test the case with a
+    // higher minor version).
+    EXPECT_NO_THROW(SQLite3Accessor(SQLITE_DBFILE_NEW_MINOR_SCHEMA, "IN"));
+}
+
 // Test we can create the schema on the fly
 TEST(SQLite3Open, memoryDB) {
     EXPECT_NO_THROW(SQLite3Accessor accessor(SQLITE_DBFILE_MEMORY, "IN"));
@@ -1297,17 +1318,4 @@ TEST_F(SQLite3Update, addDiffWithUpdate) {
 
     checkDiffs(expected_stored, accessor->getDiffs(zone_id, 1234, 1300));
 }
-
-TEST_F(SQLite3Update, addDiffWithNoTable) {
-    // An attempt of adding diffs to an old version of database that doesn't
-    // have a diffs table.  This will fail in preparing the statement.
-    initAccessor(SQLITE_DBFILE_EXAMPLE + ".nodiffs", "IN");
-    zone_id = accessor->startUpdateZone("example.com.", false).second;
-    copy(diff_begin_data, diff_begin_data + DatabaseAccessor::DIFF_PARAM_COUNT,
-         diff_params);
-    EXPECT_THROW(accessor->addRecordDiff(zone_id, getVersion(diff_begin_data),
-                                         getOperation(diff_begin_data),
-                                         diff_params),
-                 SQLite3Error);
-}
 } // end anonymous namespace

src/lib/datasrc/tests/sqlite3_unittest.cc

@@ -51,6 +51,10 @@ ConstElementPtr SQLITE_DBFILE_BROKENDB = Element::fromJSON(
     "{ \"database_file\": \"" TEST_DATA_DIR "/brokendb.sqlite3\"}");
 ConstElementPtr SQLITE_DBFILE_MEMORY = Element::fromJSON(
     "{ \"database_file\": \":memory:\"}");
+ConstElementPtr SQLITE_DBFILE_NEWSCHEMA = Element::fromJSON(
+    "{ \"database_file\": \"" TEST_DATA_DIR "/newschema.sqlite3\"}");
+ConstElementPtr SQLITE_DBFILE_OLDSCHEMA = Element::fromJSON(
+    "{ \"database_file\": \"" TEST_DATA_DIR "/oldschema.sqlite3\"}");
 
 // The following file must be non existent and must be non"creatable";
 // the sqlite3 library will try to create a new DB file if it doesn't exist,
@@ -403,6 +407,17 @@ TEST_F(Sqlite3DataSourceTest, openBrokenDB) {
     EXPECT_EQ(DataSrc::SUCCESS, data_source.init(SQLITE_DBFILE_EXAMPLE));
 }
 
+// Different schema versions, see sqlite3_accessor_unittest.
+TEST_F(Sqlite3DataSourceTest, differentSchemaVersions) {
+    EXPECT_EQ(DataSrc::SUCCESS, data_source.close());
+    EXPECT_THROW(data_source.init(SQLITE_DBFILE_NEWSCHEMA),
+                 IncompatibleDbVersion);
+    EXPECT_THROW(data_source.init(SQLITE_DBFILE_OLDSCHEMA),
+                 IncompatibleDbVersion);
+    // Don't bother to test the new_minor case; we should retire this stuff
+    // before it really happens.
+}
+
 // This test only confirms that on-the-fly schema creation works.
 TEST_F(Sqlite3DataSourceTest, memoryDB) {
     EXPECT_EQ(DataSrc::SUCCESS, data_source.close());

src/lib/python/isc/datasrc/sqlite3_ds.py

@@ -23,6 +23,10 @@ RR_NAME_INDEX = 2
 RR_TTL_INDEX = 4
 RR_RDATA_INDEX = 7
 
+# Current major and minor versions of schema
+SCHEMA_MAJOR_VERSION = 2
+SCHEMA_MINOR_VERSION = 0
+
 class Sqlite3DSError(Exception):
     """ Define exceptions."""
     pass
@@ -47,40 +51,46 @@ def create(cur):
         cur.execute("SELECT version FROM schema_version")
         row = cur.fetchone()
     except sqlite3.OperationalError:
-        cur.execute("CREATE TABLE schema_version (version INTEGER NOT NULL)")
-        cur.execute("INSERT INTO schema_version VALUES (1)")
+        cur.execute("""CREATE TABLE schema_version (version INTEGER NOT NULL,
+                    minor INTEGER NOT NULL DEFAULT 0)""")
+        cur.execute("INSERT INTO schema_version VALUES (" +
+                    str(SCHEMA_MAJOR_VERSION) + ", " +
+                    str(SCHEMA_MINOR_VERSION) + ")")
         cur.execute("""CREATE TABLE zones (id INTEGER PRIMARY KEY,
-                    name STRING NOT NULL COLLATE NOCASE,
-                    rdclass STRING NOT NULL COLLATE NOCASE DEFAULT 'IN',
+                    name TEXT NOT NULL COLLATE NOCASE,
+                    rdclass TEXT NOT NULL COLLATE NOCASE DEFAULT 'IN',
                     dnssec BOOLEAN NOT NULL DEFAULT 0)""")
         cur.execute("CREATE INDEX zones_byname ON zones (name)")
         cur.execute("""CREATE TABLE records (id INTEGER PRIMARY KEY,
                     zone_id INTEGER NOT NULL,
-                    name STRING NOT NULL COLLATE NOCASE,
-                    rname STRING NOT NULL COLLATE NOCASE,
+                    name TEXT NOT NULL COLLATE NOCASE,
+                    rname TEXT NOT NULL COLLATE NOCASE,
                     ttl INTEGER NOT NULL,
-                    rdtype STRING NOT NULL COLLATE NOCASE,
-                    sigtype STRING COLLATE NOCASE,
-                    rdata STRING NOT NULL)""")
+                    rdtype TEXT NOT NULL COLLATE NOCASE,
+                    sigtype TEXT COLLATE NOCASE,
+                    rdata TEXT NOT NULL)""")
         cur.execute("CREATE INDEX records_byname ON records (name)")
         cur.execute("CREATE INDEX records_byrname ON records (rname)")
+        cur.execute("""CREATE INDEX records_bytype_and_rname ON records
+                       (rdtype, rname)""")
         cur.execute("""CREATE TABLE nsec3 (id INTEGER PRIMARY KEY,
                     zone_id INTEGER NOT NULL,
-                    hash STRING NOT NULL COLLATE NOCASE,
-                    owner STRING NOT NULL COLLATE NOCASE,
+                    hash TEXT NOT NULL COLLATE NOCASE,
+                    owner TEXT NOT NULL COLLATE NOCASE,
                     ttl INTEGER NOT NULL,
-                    rdtype STRING NOT NULL COLLATE NOCASE,
-                    rdata STRING NOT NULL)""")
+                    rdtype TEXT NOT NULL COLLATE NOCASE,
+                    rdata TEXT NOT NULL)""")
         cur.execute("CREATE INDEX nsec3_byhash ON nsec3 (hash)")
         cur.execute("""CREATE TABLE diffs (id INTEGER PRIMARY KEY,
                     zone_id INTEGER NOT NULL,
                     version INTEGER NOT NULL,
                     operation INTEGER NOT NULL,
-                    name STRING NOT NULL COLLATE NOCASE,
-                    rrtype STRING NOT NULL COLLATE NOCASE,
+                    name TEXT NOT NULL COLLATE NOCASE,
+                    rrtype TEXT NOT NULL COLLATE NOCASE,
                     ttl INTEGER NOT NULL,
-                    rdata STRING NOT NULL)""")
-        row = [1]
+                    rdata TEXT NOT NULL)""")
+        cur.execute("SELECT version FROM schema_version")
+        row = cur.fetchone()
     cur.execute("COMMIT TRANSACTION")
     return row
 
@@ -115,8 +125,9 @@ def open(dbfile, connect_timeout=5.0):
         row = create(cur)
         conn.isolation_level = iso_lvl
 
-    if row == None or row[0] != 1:
-        raise Sqlite3DSError("Bad database schema version")
+    if row == None or row[0] != SCHEMA_MAJOR_VERSION:
+        bad_version = "(unknown)" if row is None else str(row[0])
+        raise Sqlite3DSError("Bad database schema version: " + bad_version)
 
     return conn, cur
 

src/lib/python/isc/datasrc/tests/Makefile.am

@@ -1,12 +1,14 @@
 PYCOVERAGE_RUN = @PYCOVERAGE_RUN@
 # old tests, TODO remove or change to use new API?
-#PYTESTS = master_test.py sqlite3_ds_test.py
-PYTESTS =  datasrc_test.py
+#PYTESTS = master_test.py
+PYTESTS =  datasrc_test.py sqlite3_ds_test.py
 EXTRA_DIST = $(PYTESTS)
 
 EXTRA_DIST += testdata/brokendb.sqlite3
 EXTRA_DIST += testdata/example.com.sqlite3
-EXTRA_DIST += testdata/test.sqlite3.nodiffs
+EXTRA_DIST += testdata/newschema.sqlite3
+EXTRA_DIST += testdata/oldschema.sqlite3
+EXTRA_DIST += testdata/new_minor_schema.sqlite3
 CLEANFILES = $(abs_builddir)/rwtest.sqlite3.copied
 
 # If necessary (rare cases), explicitly specify paths to dynamic libraries

src/lib/python/isc/datasrc/tests/datasrc_test.py

@@ -880,15 +880,6 @@ class JournalRead(unittest.TestCase):
         # ZoneJournalReader can only be constructed via a factory
         self.assertRaises(TypeError, ZoneJournalReader)
 
-    def test_journal_reader_old_schema(self):
-        # The database doesn't have a "diffs" table.
-        dbfile = TESTDATA_PATH + 'test.sqlite3.nodiffs'
-        client = isc.datasrc.DataSourceClient("sqlite3",
-                                              "{ \"database_file\": \"" + \
-                                                  dbfile + "\" }")
-        self.assertRaises(isc.datasrc.Error, client.get_journal_reader,
-                          self.zname, 0, 1)
-
 if __name__ == "__main__":
     isc.log.init("bind10")
     isc.log.resetUnitTestRootLogger()

src/lib/python/isc/datasrc/tests/sqlite3_ds_test.py

@@ -22,122 +22,18 @@ import sqlite3
 TESTDATA_PATH = os.environ['TESTDATA_PATH'] + os.sep
 TESTDATA_WRITE_PATH = os.environ['TESTDATA_WRITE_PATH'] + os.sep
 
-READ_ZONE_DB_FILE = TESTDATA_PATH + "example.com.sqlite3"
-BROKEN_DB_FILE = TESTDATA_PATH + "brokendb.sqlite3"
-WRITE_ZONE_DB_FILE = TESTDATA_WRITE_PATH + "example.com.out.sqlite3"
-NEW_DB_FILE = TESTDATA_WRITE_PATH + "new_db.sqlite3"
-
-def example_reader():
-    my_zone = [
-        ("example.com.",    "3600",    "IN",  "SOA", "ns.example.com. admin.example.com. 1234 3600 1800 2419200 7200"),
-        ("example.com.",    "3600",    "IN",  "NS", "ns.example.com."),
-        ("ns.example.com.", "3600",    "IN",  "A", "192.0.2.1")
-    ]
-    for rr in my_zone:
-        yield rr
-
-def example_reader_nested():
-    # this iterator is used in the 'locked' test; it will cause
-    # the load() method to try and write to the same database
-    sqlite3_ds.load(WRITE_ZONE_DB_FILE,
-                    ".",
-                    example_reader)
-    return example_reader()
-
-class TestSqlite3_ds(unittest.TestCase):
-    def test_zone_exist(self):
-        # The following file must be non existent and must be non
-        # "creatable"; the sqlite3 library will try to create a new
-        # DB file if it doesn't exist, so to test a failure case the
-        # create operation should also fail. The "nodir", a non
-        # existent directory, is inserted for this purpose.
-        nodir = "/nodir/notexist"
-        self.assertRaises(sqlite3_ds.Sqlite3DSError,
-                          sqlite3_ds.zone_exist, "example.com", nodir)
-        # Open a broken database file
-        self.assertRaises(sqlite3_ds.Sqlite3DSError,
-                          sqlite3_ds.zone_exist, "example.com",
-                          BROKEN_DB_FILE)
-        self.assertTrue(sqlite3_ds.zone_exist("example.com.",
-                        READ_ZONE_DB_FILE))
-        self.assertFalse(sqlite3_ds.zone_exist("example.org.",
-                         READ_ZONE_DB_FILE))
-
-    def test_load_db(self):
-        sqlite3_ds.load(WRITE_ZONE_DB_FILE, ".", example_reader)
-
-    def test_locked_db(self):
-        # load it first to make sure it exists
-        sqlite3_ds.load(WRITE_ZONE_DB_FILE, ".", example_reader)
-
-        # and manually create a writing session as well
-        con = sqlite3.connect(WRITE_ZONE_DB_FILE);
-        cur = con.cursor()
-        cur.execute("delete from records")
-
-        self.assertRaises(sqlite3_ds.Sqlite3DSError,
-                          sqlite3_ds.load, WRITE_ZONE_DB_FILE, ".",
-                          example_reader)
-
-        con.rollback()
-
-        # and make sure lock does not stay
-        sqlite3_ds.load(WRITE_ZONE_DB_FILE, ".", example_reader)
-
-        # force locked db by nested loads
-        self.assertRaises(sqlite3_ds.Sqlite3DSError,
-                          sqlite3_ds.load, WRITE_ZONE_DB_FILE, ".",
-                          example_reader_nested)
-
-        # and make sure lock does not stay
-        sqlite3_ds.load(WRITE_ZONE_DB_FILE, ".", example_reader)
+DBFILE_NEWSCHEMA = TESTDATA_PATH + "/newschema.sqlite3";
+DBFILE_OLDSCHEMA = TESTDATA_PATH + "/oldschema.sqlite3";
+DBFILE_NEW_MINOR_SCHEMA = TESTDATA_PATH + "/new_minor_schema.sqlite3";
 
 class NewDBFile(unittest.TestCase):
-    def tearDown(self):
-        # remove the created database after every test
-        if (os.path.exists(NEW_DB_FILE)):
-            os.remove(NEW_DB_FILE)
-
-    def setUp(self):
-        # remove the created database before every test too, just
-        # in case a test got aborted half-way, and cleanup didn't occur
-        if (os.path.exists(NEW_DB_FILE)):
-            os.remove(NEW_DB_FILE)
-
-    def test_new_db(self):
-        self.assertFalse(os.path.exists(NEW_DB_FILE))
-        sqlite3_ds.open(NEW_DB_FILE)
-        self.assertTrue(os.path.exists(NEW_DB_FILE))
-
-    def test_new_db_locked(self):
-        self.assertFalse(os.path.exists(NEW_DB_FILE))
-        con = sqlite3.connect(NEW_DB_FILE);
-        con.isolation_level = None
-        cur = con.cursor()
-        cur.execute("BEGIN IMMEDIATE TRANSACTION")
-
-        # load should now fail, since the database is locked,
-        # and the open() call needs an exclusive lock
-        self.assertRaises(sqlite3.OperationalError,
-                          sqlite3_ds.open, NEW_DB_FILE, 0.1)
-
-        con.rollback()
-        cur.close()
-        con.close()
-        self.assertTrue(os.path.exists(NEW_DB_FILE))
-
-        # now that we closed our connection, load should work again
-        sqlite3_ds.open(NEW_DB_FILE)
-
-        # the database should now have been created, and a new load should
-        # not require an exclusive lock anymore, so we lock it again
-        con = sqlite3.connect(NEW_DB_FILE);
-        cur = con.cursor()
-        cur.execute("BEGIN IMMEDIATE TRANSACTION")
-        sqlite3_ds.open(NEW_DB_FILE, 0.1)
-        con.rollback()
-        cur.close()
-        con.close()
+    def test_different_version(self):
+        self.assertTrue(os.path.exists(DBFILE_NEWSCHEMA))
+        self.assertRaises(sqlite3_ds.Sqlite3DSError, sqlite3_ds.open,
+                          DBFILE_NEWSCHEMA)
+        self.assertRaises(sqlite3_ds.Sqlite3DSError, sqlite3_ds.open,
+                          DBFILE_OLDSCHEMA)
+        self.assertNotEqual(None, sqlite3_ds.open(DBFILE_NEW_MINOR_SCHEMA)[0])
 
 if __name__ == '__main__':
     unittest.main()