[1287] added generic startTransaction() method

src/lib/datasrc/database.h

@@ -260,13 +260,14 @@ public:
     /// \c commitUpdateZone()); if it's false, the existing records will be
     /// intact unless explicitly deleted by \c deleteRecordInZone().
     ///
-    /// A single \c DatabaseAccessor instance can perform at most one update
+    /// A single \c DatabaseAccessor instance can perform at most one
     /// transaction; a duplicate call to this method before
-    /// \c commitUpdateZone() or \c rollbackUpdateZone() will result in
-    /// a \c DataSourceError exception.  If multiple update attempts need
-    /// to be performed concurrently (and if the underlying database allows
-    /// such operation), separate \c DatabaseAccessor instance must be
-    /// created.
+    /// \c commitUpdateZone() or \c rollbackUpdateZone(), or a call to this
+    /// method within another transaction started by \c startTransaction()
+    /// will result in a \c DataSourceError exception.
+    /// If multiple update attempts need to be performed concurrently (and
+    /// if the underlying database allows such operation), separate
+    /// \c DatabaseAccessor instance must be created.
     ///
     /// \note The underlying database may not allow concurrent updates to
     /// the same database instance even if different "connections" (or
@@ -295,8 +296,9 @@ public:
     /// \c getZone(); for example, a specific implementation may use a
     /// completely new zone ID when \c replace is true.
     ///
-    /// \exception DataSourceError Duplicate call to this method, or some
-    /// internal database related error.
+    /// \exception DataSourceError Duplicate call to this method, call to
+    /// this method within another transaction, or some internal database
+    /// related error.
     ///
     /// \param zone_name A string representation of the zone name to be updated
     /// \param replace Whether to replace the entire zone (see above)
@@ -382,6 +384,26 @@ public:
     virtual void deleteRecordInZone(
         const std::string (&params)[DEL_PARAM_COUNT]) = 0;
 
+    /// Start a general transaction.
+    ///
+    /// Each derived class version of this method starts a database
+    /// transaction in a way specific to the database details.  Any subsequent
+    /// operations on the accessor are guaranteed to be not susceptible to
+    /// any update attempts made during the transaction.  The transaction
+    /// must be terminated by either \c commit() or \c rollback().
+    ///
+    /// In practice, this transaction is intended to be used to perform
+    /// a set of atomic reads and work as a read-only lock.  So, in many
+    /// cases \c commit() and \c rollback() will have the same effect.
+    ///
+    /// This transaction cannot coexist with an update transaction started
+    /// by \c startUpdateZone().  Such an attempt will result in
+    /// \c DataSourceError.
+    ///
+    /// \exception DataSourceError An attempt of nested transaction, or some
+    /// internal database related error.
+    virtual void startTransaction() = 0;
+
     /// Commit a transaction.
     ///
     /// This method completes a transaction started by \c startTransaction

src/lib/datasrc/sqlite3_accessor.cc

@@ -541,10 +541,14 @@ SQLite3Accessor::getAllRecords(int id) const {
 
 pair<bool, int>
 SQLite3Accessor::startUpdateZone(const string& zone_name, const bool replace) {
-    if (dbparameters_->in_transaction || dbparameters_->updating_zone) {
+    if (dbparameters_->updating_zone) {
         isc_throw(DataSourceError,
                   "duplicate zone update on SQLite3 data source");
     }
+    if (dbparameters_->in_transaction) {
+        isc_throw(DataSourceError,
+                  "zone update attempt in another SQLite3 transaction");
+    }
 
     const pair<bool, int> zone_info(getZone(zone_name));
     if (!zone_info.first) {
@@ -552,7 +556,7 @@ SQLite3Accessor::startUpdateZone(const string& zone_name, const bool replace) {
     }
 
     StatementProcessor(*dbparameters_, BEGIN,
-                       "start an SQLite3 transaction").exec();
+                       "start an SQLite3 update transaction").exec();
 
     if (replace) {
         try {
@@ -587,6 +591,18 @@ SQLite3Accessor::startUpdateZone(const string& zone_name, const bool replace) {
 }
 
 void
+SQLite3Accessor::startTransaction() {
+    if (dbparameters_->in_transaction) {
+        isc_throw(DataSourceError,
+                  "duplicate transaction on SQLite3 data source");
+    }
+
+    StatementProcessor(*dbparameters_, BEGIN,
+                       "start an SQLite3 transaction").exec();
+    dbparameters_->in_transaction = true;
+}
+
+void
 SQLite3Accessor::commit() {
     if (!dbparameters_->in_transaction) {
         isc_throw(DataSourceError, "performing commit on SQLite3 "

src/lib/datasrc/sqlite3_accessor.h

@@ -131,6 +131,8 @@ public:
     virtual std::pair<bool, int> startUpdateZone(const std::string& zone_name,
                                                  bool replace);
 
+    virtual void startTransaction();
+
     /// \note we are quite impatient here: it's quite possible that the COMMIT
     /// fails due to other process performing SELECT on the same database
     /// (consider the case where COMMIT is done by xfrin or dynamic update

src/lib/datasrc/tests/database_unittest.cc

@@ -223,6 +223,7 @@ public:
         // return dummy value.  unused anyway.
         return (pair<bool, int>(true, 0));
     }
+    virtual void startTransaction() {}
     virtual void commit() {}
     virtual void rollback() {}
     virtual void addRecordToZone(const string (&)[ADD_COLUMN_COUNT]) {}

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -768,4 +768,80 @@ TEST_F(SQLite3Update, invalidDelete) {
     // An attempt of delete before an explicit start of transaction
     EXPECT_THROW(accessor->deleteRecordInZone(del_params), DataSourceError);
 }
+
+TEST_F(SQLite3Update, emptyTransaction) {
+    // A generic transaction without doing anything inside it.  Just check
+    // it doesn't throw or break the database.
+    checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
+    accessor->startTransaction();
+    checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
+    accessor->commit();
+    checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
+}
+
+TEST_F(SQLite3Update, duplicateTransaction) {
+    accessor->startTransaction();
+    EXPECT_THROW(accessor->startTransaction(), DataSourceError);
+}
+
+TEST_F(SQLite3Update, transactionInUpdate) {
+    accessor->startUpdateZone("example.com.", true);
+    EXPECT_THROW(accessor->startTransaction(), DataSourceError);
+}
+
+TEST_F(SQLite3Update, updateInTransaction) {
+    accessor->startTransaction();
+    EXPECT_THROW(accessor->startUpdateZone("example.com.", true),
+                 DataSourceError);
+}
+
+TEST_F(SQLite3Update, updateWithTransaction) {
+    // Start a read-only transaction, wherein we execute two reads.
+    // Meanwhile we start a write (update) transaction.  The commit attempt
+    // for the write transaction will due to the lock held by the read
+    // transaction.  The database should be intact.
+    another_accessor->startTransaction();
+    checkRecords(*another_accessor, zone_id, "foo.bar.example.com.",
+                 expected_stored);
+
+    ASSERT_TRUE(accessor->startUpdateZone("example.com.", true).first);
+    EXPECT_THROW(accessor->commit(), DataSourceError);
+
+    checkRecords(*another_accessor, zone_id, "foo.bar.example.com.",
+                 expected_stored);
+    another_accessor->commit(); // this shouldn't throw
+}
+
+TEST_F(SQLite3Update, updateWithoutTransaction) {
+    // Similar to the previous test, but reads are not protected in a
+    // transaction.  So the write transaction will succeed and flush the DB,
+    // and the result of the second read is different from the first.
+    checkRecords(*another_accessor, zone_id, "foo.bar.example.com.",
+                 expected_stored);
+
+    ASSERT_TRUE(accessor->startUpdateZone("example.com.", true).first);
+    accessor->commit();
+
+    checkRecords(*another_accessor, zone_id, "foo.bar.example.com.",
+                 empty_stored);
+}
+
+TEST_F(SQLite3Update, concurrentTransactions) {
+    // Two read-only transactions coexist (unlike the read vs write)
+    // Start one transaction.
+    accessor->startTransaction();
+    checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
+
+    // Start a new one.
+    another_accessor->startTransaction();
+
+    // The second transaction doesn't affect the first or vice versa.
+    checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
+    checkRecords(*another_accessor, zone_id, "foo.bar.example.com.",
+                 expected_stored);
+
+    // Commit should be successful for both transactions.
+    accessor->commit();
+    another_accessor->commit();
+}
 } // end anonymous namespace