Merge branch 'trac326'

src/lib/datasrc/sqlite3_accessor.cc

@@ -21,6 +21,8 @@
 
 #include <boost/lexical_cast.hpp>
 
+#define SQLITE_SCHEMA_VERSION 1
+
 namespace isc {
 namespace datasrc {
 
@@ -121,6 +123,8 @@ const char* const SCHEMA_LIST[] = {
     NULL
 };
 
+const char* const q_version_str = "SELECT version FROM schema_version";
+
 const char* const q_zone_str = "SELECT id FROM zones WHERE name=?1 AND rdclass = ?2";
 
 // note that the order of the SELECT values is specifically chosen to match
@@ -177,29 +181,90 @@ prepare(sqlite3* const db, const char* const statement) {
     return (prepared);
 }
 
-void
+// small function to sleep for 0.1 seconds, needed when waiting for
-checkAndSetupSchema(Initializer* initializer) {
+// exclusive database locks (which should only occur on startup, and only
-    sqlite3* const db = initializer->params_.db_;
+// when the database has not been created yet)
+void do_sleep() {
+    struct timespec req;
+    req.tv_sec = 0;
+    req.tv_nsec = 100000000;
+    nanosleep(&req, NULL);
+}
 
+// returns the schema version if the schema version table exists
+// returns -1 if it does not
+int check_schema_version(sqlite3* db) {
     sqlite3_stmt* prepared = NULL;
-    if (sqlite3_prepare_v2(db, "SELECT version FROM schema_version", -1,
+    // At this point in time, the database might be exclusively locked, in
-                           &prepared, NULL) == SQLITE_OK &&
+    // which case even prepare() will return BUSY, so we may need to try a
-        sqlite3_step(prepared) == SQLITE_ROW) {
+    // few times
-        initializer->params_.version_ = sqlite3_column_int(prepared, 0);
+    for (size_t i = 0; i < 50; ++i) {
-        sqlite3_finalize(prepared);
+        int rc = sqlite3_prepare_v2(db, q_version_str, -1, &prepared, NULL);
-    } else {
+        if (rc == SQLITE_ERROR) {
-        logger.info(DATASRC_SQLITE_SETUP);
+            // this is the error that is returned when the table does not
-        if (prepared != NULL) {
+            // exist
-            sqlite3_finalize(prepared);
+            return (-1);
+        } else if (rc == SQLITE_OK) {
+            break;
+        } else if (rc != SQLITE_BUSY || i == 50) {
+            isc_throw(SQLite3Error, "Unable to prepare version query: "
+                        << rc << " " << sqlite3_errmsg(db));
+        }
+        do_sleep();
+    }
+    if (sqlite3_step(prepared) != SQLITE_ROW) {
+        isc_throw(SQLite3Error,
+                    "Unable to query version: " << sqlite3_errmsg(db));
+    }
+    int version = sqlite3_column_int(prepared, 0);
+    sqlite3_finalize(prepared);
+    return (version);
+}
+
+// return db version
+int create_database(sqlite3* db) {
+    // 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) {
         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);
+    }
+}
+
+void
+checkAndSetupSchema(Initializer* initializer) {
+    sqlite3* const db = initializer->params_.db_;
+
+    int schema_version = check_schema_version(db);
+    if (schema_version != SQLITE_SCHEMA_VERSION) {
+        schema_version = create_database(db);
     }
+    initializer->params_.version_ = schema_version;
 
     initializer->params_.q_zone_ = prepare(db, q_zone_str);
     /* TODO: Yet unneeded statements

src/lib/datasrc/sqlite3_datasrc.cc

@@ -26,6 +26,8 @@
 #include <dns/rrset.h>
 #include <dns/rrsetlist.h>
 
+#define SQLITE_SCHEMA_VERSION 1
+
 using namespace std;
 using namespace isc::dns;
 using namespace isc::dns::rdata;
@@ -77,6 +79,8 @@ const char* const SCHEMA_LIST[] = {
     NULL
 };
 
+const char* const q_version_str = "SELECT version FROM schema_version";
+
 const char* const q_zone_str = "SELECT id FROM zones WHERE name=?1";
 
 const char* const q_record_str = "SELECT rdtype, ttl, sigtype, rdata "
@@ -254,7 +258,7 @@ Sqlite3DataSrc::findRecords(const Name& name, const RRType& rdtype,
         }
         break;
     }
-    
+
     sqlite3_reset(query);
     sqlite3_clear_bindings(query);
 
@@ -295,7 +299,7 @@ Sqlite3DataSrc::findRecords(const Name& name, const RRType& rdtype,
     //
     sqlite3_reset(dbparameters->q_count_);
     sqlite3_clear_bindings(dbparameters->q_count_);
-    
+
     rc = sqlite3_bind_int(dbparameters->q_count_, 1, zone_id);
     if (rc != SQLITE_OK) {
         isc_throw(Sqlite3Error, "Could not bind zone ID " << zone_id <<
@@ -653,29 +657,90 @@ prepare(sqlite3* const db, const char* const statement) {
     return (prepared);
 }
 
-void
+// small function to sleep for 0.1 seconds, needed when waiting for
-checkAndSetupSchema(Sqlite3Initializer* initializer) {
+// exclusive database locks (which should only occur on startup, and only
-    sqlite3* const db = initializer->params_.db_;
+// when the database has not been created yet)
+void do_sleep() {
+    struct timespec req;
+    req.tv_sec = 0;
+    req.tv_nsec = 100000000;
+    nanosleep(&req, NULL);
+}
 
+// returns the schema version if the schema version table exists
+// returns -1 if it does not
+int check_schema_version(sqlite3* db) {
     sqlite3_stmt* prepared = NULL;
-    if (sqlite3_prepare_v2(db, "SELECT version FROM schema_version", -1,
+    // At this point in time, the database might be exclusively locked, in
-                           &prepared, NULL) == SQLITE_OK &&
+    // which case even prepare() will return BUSY, so we may need to try a
-        sqlite3_step(prepared) == SQLITE_ROW) {
+    // few times
-        initializer->params_.version_ = sqlite3_column_int(prepared, 0);
+    for (size_t i = 0; i < 50; ++i) {
-        sqlite3_finalize(prepared);
+        int rc = sqlite3_prepare_v2(db, q_version_str, -1, &prepared, NULL);
-    } else {
+        if (rc == SQLITE_ERROR) {
-        logger.info(DATASRC_SQLITE_SETUP);
+            // this is the error that is returned when the table does not
-        if (prepared != NULL) {
+            // exist
-            sqlite3_finalize(prepared);
+            return (-1);
+        } else if (rc == SQLITE_OK) {
+            break;
+        } else if (rc != SQLITE_BUSY || i == 50) {
+            isc_throw(Sqlite3Error, "Unable to prepare version query: "
+                        << rc << " " << sqlite3_errmsg(db));
         }
+        do_sleep();
+    }
+    if (sqlite3_step(prepared) != SQLITE_ROW) {
+        isc_throw(Sqlite3Error,
+                    "Unable to query version: " << sqlite3_errmsg(db));
+    }
+    int version = sqlite3_column_int(prepared, 0);
+    sqlite3_finalize(prepared);
+    return (version);
+}
+
+// return db version
+int create_database(sqlite3* db) {
+    // 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) {
         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);
+    }
+}
+
+void
+checkAndSetupSchema(Sqlite3Initializer* initializer) {
+    sqlite3* const db = initializer->params_.db_;
+
+    int schema_version = check_schema_version(db);
+    if (schema_version != SQLITE_SCHEMA_VERSION) {
+        schema_version = create_database(db);
     }
+    initializer->params_.version_ = schema_version;
 
     initializer->params_.q_zone_ = prepare(db, q_zone_str);
     initializer->params_.q_record_ = prepare(db, q_record_str);

src/lib/datasrc/tests/Makefile.am

@@ -3,6 +3,7 @@ AM_CPPFLAGS += -I$(top_builddir)/src/lib/dns -I$(top_srcdir)/src/lib/dns
 AM_CPPFLAGS += $(BOOST_INCLUDES)
 AM_CPPFLAGS += $(SQLITE_CFLAGS)
 AM_CPPFLAGS += -DTEST_DATA_DIR=\"$(srcdir)/testdata\"
+AM_CPPFLAGS += -DTEST_DATA_BUILD_DIR=\"$(builddir)/testdata\"
 
 AM_CXXFLAGS = $(B10_CXXFLAGS)
 

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -19,6 +19,8 @@
 
 #include <gtest/gtest.h>
 #include <boost/scoped_ptr.hpp>
+#include <fstream>
+#include <sqlite3.h>
 
 using namespace isc::datasrc;
 using isc::data::ConstElementPtr;
@@ -43,6 +45,10 @@ std::string SQLITE_DBFILE_EXAMPLE_ORG = TEST_DATA_DIR "/example.org.sqlite3";
 // The "nodir", a non existent directory, is inserted for this purpose.
 std::string SQLITE_DBFILE_NOTEXIST = TEST_DATA_DIR "/nodir/notexist";
 
+// new db file, we don't need this to be a std::string, and given the
+// raw calls we use it in a const char* is more convenient
+const char* SQLITE_NEW_DBFILE = TEST_DATA_BUILD_DIR "/newdb.sqlite3";
+
 // Opening works (the content is tested in different tests)
 TEST(SQLite3Open, common) {
     EXPECT_NO_THROW(SQLite3Database db(SQLITE_DBFILE_EXAMPLE,
@@ -339,4 +345,63 @@ TEST_F(SQLite3Access, getRecords) {
     EXPECT_FALSE(context->getNext(columns));
 }
 
+// Test fixture for creating a db that automatically deletes it before start,
+// and when done
+class SQLite3Create : public ::testing::Test {
+public:
+    SQLite3Create() {
+        remove(SQLITE_NEW_DBFILE);
+    }
+
+    ~SQLite3Create() {
+        remove(SQLITE_NEW_DBFILE);
+    }
+};
+
+bool exists(const char* filename) {
+    std::ifstream f(filename);
+    return (f != NULL);
+}
+
+TEST_F(SQLite3Create, creationtest) {
+    ASSERT_FALSE(exists(SQLITE_NEW_DBFILE));
+    // Should simply be created
+    SQLite3Database db(SQLITE_NEW_DBFILE, RRClass::IN());
+    ASSERT_TRUE(exists(SQLITE_NEW_DBFILE));
+}
+
+TEST_F(SQLite3Create, emptytest) {
+    ASSERT_FALSE(exists(SQLITE_NEW_DBFILE));
+
+    // open one manualle
+    sqlite3* db;
+    ASSERT_EQ(SQLITE_OK, sqlite3_open(SQLITE_NEW_DBFILE, &db));
+
+    // empty, but not locked, so creating it now should work
+    SQLite3Database db2(SQLITE_NEW_DBFILE, RRClass::IN());
+
+    sqlite3_close(db);
+
+    // should work now that we closed it
+    SQLite3Database db3(SQLITE_NEW_DBFILE, RRClass::IN());
+}
+
+TEST_F(SQLite3Create, lockedtest) {
+    ASSERT_FALSE(exists(SQLITE_NEW_DBFILE));
+
+    // open one manually
+    sqlite3* db;
+    ASSERT_EQ(SQLITE_OK, sqlite3_open(SQLITE_NEW_DBFILE, &db));
+    sqlite3_exec(db, "BEGIN EXCLUSIVE TRANSACTION", NULL, NULL, NULL);
+
+    // should not be able to open it
+    EXPECT_THROW(SQLite3Database db2(SQLITE_NEW_DBFILE, RRClass::IN()),
+                 SQLite3Error);
+
+    sqlite3_exec(db, "ROLLBACK TRANSACTION", NULL, NULL, NULL);
+
+    // should work now that we closed it
+    SQLite3Database db3(SQLITE_NEW_DBFILE, RRClass::IN());
+}
+
 } // end anonymous namespace

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

@@ -33,44 +33,63 @@ def create(cur):
     Arguments:
         cur - sqlite3 cursor.
     """
-    cur.execute("CREATE TABLE schema_version (version INTEGER NOT NULL)")
+    # We are creating the database because it apparently had not been at
-    cur.execute("INSERT INTO schema_version VALUES (1)")
+    # the time we tried to read from it. However, another process may have
-    cur.execute("""CREATE TABLE zones (id INTEGER PRIMARY KEY,
+    # had the same idea, resulting in a potential race condition.
-                   name STRING NOT NULL COLLATE NOCASE,
+    # Therefore, we obtain an exclusive lock before we create anything
-                   rdclass STRING NOT NULL COLLATE NOCASE DEFAULT 'IN',
+    # When we have it, we check *again* whether the database has been
-                   dnssec BOOLEAN NOT NULL DEFAULT 0)""")
+    # initialized. If not, we do so.
-    cur.execute("CREATE INDEX zones_byname ON zones (name)")
+
-    cur.execute("""CREATE TABLE records (id INTEGER PRIMARY KEY,
+    # If the database is perpetually locked, it'll time out automatically
-                   zone_id INTEGER NOT NULL,
+    # and we just let it fail.
-                   name STRING NOT NULL COLLATE NOCASE,
+    cur.execute("BEGIN EXCLUSIVE TRANSACTION")
-                   rname STRING NOT NULL COLLATE NOCASE,
+    try:
-                   ttl INTEGER NOT NULL,
+        cur.execute("SELECT version FROM schema_version")
-                   rdtype STRING NOT NULL COLLATE NOCASE,
+        row = cur.fetchone()
-                   sigtype STRING COLLATE NOCASE,
+    except sqlite3.OperationalError:
-                   rdata STRING NOT NULL)""")
+        cur.execute("CREATE TABLE schema_version (version INTEGER NOT NULL)")
-    cur.execute("CREATE INDEX records_byname ON records (name)")
+        cur.execute("INSERT INTO schema_version VALUES (1)")
-    cur.execute("CREATE INDEX records_byrname ON records (rname)")
+        cur.execute("""CREATE TABLE zones (id INTEGER PRIMARY KEY,
-    cur.execute("""CREATE TABLE nsec3 (id INTEGER PRIMARY KEY,
+                    name STRING NOT NULL COLLATE NOCASE,
-                   zone_id INTEGER NOT NULL,
+                    rdclass STRING NOT NULL COLLATE NOCASE DEFAULT 'IN',
-                   hash STRING NOT NULL COLLATE NOCASE,
+                    dnssec BOOLEAN NOT NULL DEFAULT 0)""")
-                   owner STRING NOT NULL COLLATE NOCASE,
+        cur.execute("CREATE INDEX zones_byname ON zones (name)")
-                   ttl INTEGER NOT NULL,
+        cur.execute("""CREATE TABLE records (id INTEGER PRIMARY KEY,
-                   rdtype STRING NOT NULL COLLATE NOCASE,
+                    zone_id INTEGER NOT NULL,
-                   rdata STRING NOT NULL)""")
+                    name STRING NOT NULL COLLATE NOCASE,
-    cur.execute("CREATE INDEX nsec3_byhash ON nsec3 (hash)")
+                    rname STRING NOT NULL COLLATE NOCASE,
-
+                    ttl INTEGER NOT NULL,
-def open(dbfile):
+                    rdtype STRING NOT NULL COLLATE NOCASE,
+                    sigtype STRING COLLATE NOCASE,
+                    rdata STRING NOT NULL)""")
+        cur.execute("CREATE INDEX records_byname ON records (name)")
+        cur.execute("CREATE INDEX records_byrname ON records (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,
+                    ttl INTEGER NOT NULL,
+                    rdtype STRING NOT NULL COLLATE NOCASE,
+                    rdata STRING NOT NULL)""")
+        cur.execute("CREATE INDEX nsec3_byhash ON nsec3 (hash)")
+        row = [1]
+    cur.execute("COMMIT TRANSACTION")
+    return row
+
+def open(dbfile, connect_timeout=5.0):
     """ Open a database, if the database is not yet set up, call create
     to do so. It may raise Sqlite3DSError if failed to open sqlite3
     database file or find bad database schema version in the database.
 
     Arguments:
         dbfile - the filename for the sqlite3 database.
+        connect_timeout - timeout for opening the database or acquiring locks
+                          defaults to sqlite3 module's default of 5.0 seconds
 
     Return sqlite3 connection, sqlite3 cursor.
     """
     try:
-        conn = sqlite3.connect(dbfile)
+        conn = sqlite3.connect(dbfile, timeout=connect_timeout)
         cur = conn.cursor()
     except Exception as e:
         fail = "Failed to open " + dbfile + ": " + e.args[0]
@@ -80,10 +99,13 @@ def open(dbfile):
     try:
         cur.execute("SELECT version FROM schema_version")
         row = cur.fetchone()
-    except:
+    except sqlite3.OperationalError:
-        create(cur)
+        # temporarily disable automatic transactions so
-        conn.commit()
+        # we can do our own
-        row = [1]
+        iso_lvl = conn.isolation_level
+        conn.isolation_level = None
+        row = create(cur)
+        conn.isolation_level = iso_lvl
 
     if row == None or row[0] != 1:
         raise Sqlite3DSError("Bad database schema version")

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

@@ -23,8 +23,9 @@ 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"
-WRITE_ZONE_DB_FILE = TESTDATA_WRITE_PATH + "example.com.out.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 = [
@@ -91,5 +92,52 @@ class TestSqlite3_ds(unittest.TestCase):
         # and make sure lock does not stay
         sqlite3_ds.load(WRITE_ZONE_DB_FILE, ".", example_reader)
 
+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()
+
 if __name__ == '__main__':
     unittest.main()