Browse Source

[324] make sure initial SQLite3 DB creation is exception safe.

this is not really an issue of this task, and I believe we should soon stop
creating the schema in the accessor context in the first place, but the bug
is real so I fixed it anyway.  Assuming this creation code and the older
version of implementation will be gone soon, I simply duplicated the mostly
same code in both old and new implementations.

Also, it seemed extremely difficult (I'd say it's effectively impossible) to
test these cases.  I couldn't come up with any reasonable way to test them,
so I didn't add any test.
JINMEI Tatuya 13 years ago
parent
commit
4df83ec7d1
2 changed files with 93 additions and 34 deletions
  1. 48 17
      src/lib/datasrc/sqlite3_accessor.cc
  2. 45 17
      src/lib/datasrc/sqlite3_datasrc.cc

+ 48 - 17
src/lib/datasrc/sqlite3_accessor.cc

@@ -378,36 +378,67 @@ pair<int, int> checkSchemaVersion(sqlite3* db) {
     }
 }
 
+// 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
 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();
-    }
+    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);
+        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

+ 45 - 17
src/lib/datasrc/sqlite3_datasrc.cc

@@ -743,28 +743,56 @@ pair<int, int> check_schema_version(sqlite3* db) {
     }
 }
 
+// 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
 pair<int, 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();
-    }
 
+    // try to get an exclusive lock. Once that is obtained, do the version
+    // check *again*, just in case this process was racing another
+    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) {
@@ -774,7 +802,7 @@ pair<int, int> create_database(sqlite3* db) {
                         "Failed to set up schema " << SCHEMA_LIST[i]);
             }
         }
-        sqlite3_exec(db, "COMMIT TRANSACTION", NULL, NULL, NULL);
+        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