Merge #2877

Fix SQLite backend so the deletes are not so slow.

This is the urgent part of the fix; we'll have another half of the branch
later, to clean up some details.

src/lib/datasrc/database.cc

@@ -1644,17 +1644,23 @@ DatabaseUpdater::addRRset(const AbstractRRset& rrset) {
                 { cvtr.getName(), cvtr.getType(), cvtr.getTTL(), rdata_txt };
             accessor_->addRecordDiff(zone_id_, serial_.getValue(),
                                      Accessor::DIFF_ADD, journal);
+            LOG_DEBUG(logger, DBG_TRACE_DETAILED, DATASRC_DATABASE_ADDDIFF).
+                arg(cvtr.getName()).arg(cvtr.getType()).arg(rdata_txt);
         }
         if (nsec3_type) {
             const string nsec3_columns[Accessor::ADD_NSEC3_COLUMN_COUNT] =
                 { cvtr.getNSEC3Name(), cvtr.getTTL(), cvtr.getType(),
                   rdata_txt };
             accessor_->addNSEC3RecordToZone(nsec3_columns);
+            LOG_DEBUG(logger, DBG_TRACE_DETAILED, DATASRC_DATABASE_ADDNSEC3).
+                arg(cvtr.getNSEC3Name()).arg(rdata_txt);
         } else {
             const string columns[Accessor::ADD_COLUMN_COUNT] =
                 { cvtr.getName(), cvtr.getRevName(), cvtr.getTTL(),
                   cvtr.getType(), sigtype, rdata_txt };
             accessor_->addRecordToZone(columns);
+            LOG_DEBUG(logger, DBG_TRACE_DETAILED, DATASRC_DATABASE_ADDRR).
+                arg(cvtr.getName()).arg(cvtr.getType()).arg(rdata_txt);
         }
     }
 }
@@ -1698,14 +1704,21 @@ DatabaseUpdater::deleteRRset(const AbstractRRset& rrset) {
                 { cvtr.getName(), cvtr.getType(), cvtr.getTTL(), rdata_txt };
             accessor_->addRecordDiff(zone_id_, serial_.getValue(),
                                      Accessor::DIFF_DELETE, journal);
+            LOG_DEBUG(logger, DBG_TRACE_DETAILED, DATASRC_DATABASE_DELETEDIFF).
+                arg(cvtr.getName()).arg(cvtr.getType()).arg(rdata_txt);
         }
         const string params[Accessor::DEL_PARAM_COUNT] =
             { nsec3_type ? cvtr.getNSEC3Name() : cvtr.getName(),
-              cvtr.getType(), rdata_txt };
+              cvtr.getType(), rdata_txt,
+              nsec3_type ? cvtr.getNSEC3Name() : cvtr.getRevName() };
         if (nsec3_type) {
             accessor_->deleteNSEC3RecordInZone(params);
+            LOG_DEBUG(logger, DBG_TRACE_DETAILED, DATASRC_DATABASE_DELETENSEC3).
+                arg(cvtr.getNSEC3Name()).arg(rdata_txt);
         } else {
             accessor_->deleteRecordInZone(params);
+            LOG_DEBUG(logger, DBG_TRACE_DETAILED, DATASRC_DATABASE_DELETERR).
+                arg(cvtr.getName()).arg(cvtr.getType()).arg(rdata_txt);
         }
     }
 }

src/lib/datasrc/database.h

@@ -122,12 +122,24 @@ public:
     /// Each derived implementation of deleteRecordInZone() should expect
     /// the "params" array to be filled with the values as described in this
     /// enumeration, in this order.
+    ///
+    /// DEL_RNAME is included in case the reversed from is more convenient
+    /// for the underlying implementation to identify the record to be
+    /// deleted (reversed names are generally easier to sort, which may help
+    /// perform the search faster).  It's up to the underlying implementation
+    /// which one (or both) it uses for the search.   DEL_NAME and DEL_RNAME
+    /// are mutually convertible with the understanding of DNS names, and
+    /// in that sense redundant.  But both are provided so the underlying
+    /// implementation doesn't have to deal with DNS level concepts.
     enum DeleteRecordParams {
         DEL_NAME = 0, ///< The owner name of the record (a domain name)
                       ///< or the hash label for deleteNSEC3RecordInZone()
         DEL_TYPE = 1, ///< The RRType of the record (A/NS/TXT etc.)
         DEL_RDATA = 2, ///< Full text representation of the record's RDATA
-        DEL_PARAM_COUNT = 3 ///< Number of parameters
+        DEL_RNAME = 3, ///< As DEL_NAME, but with the labels of domain name
+                       ///< in reverse order (eg. org.example.). With NSEC3,
+                       ///< it is the same as DEL_NAME.
+        DEL_PARAM_COUNT = 4 ///< Number of parameters
     };
 
     /// \brief Operation mode when adding a record diff.

src/lib/datasrc/datasrc_messages.mes

@@ -83,11 +83,37 @@ with the content. The problem does not stop the new version from being used
 but it should still be checked and fixed. See the message to know what exactly
 is wrong with the data.
 
+% DATASRC_DATABASE_ADDDIFF updated diff table for add: %1 %2 %3
+Debug message. A difference record for adding a record to the zone is being
+appended to the difference table. The name, type and rdata of the record is
+logged.
+
+% DATASRC_DATABASE_ADDNSEC3 added NSEC3 RR: %1 %2
+Debug message. A new NSEC3 record is added to the table. The hash and the rdata
+is logged.
+
+% DATASRC_DATABASE_ADDRR added RR: %1 %2 %3
+Debug message. A new resource record is added to the table. The name, type and
+rdata is logged.
+
 % DATASRC_DATABASE_COVER_NSEC_UNSUPPORTED %1 doesn't support DNSSEC when asked for NSEC data covering %2
 The datasource tried to provide an NSEC proof that the named domain does not
 exist, but the database backend doesn't support DNSSEC. No proof is included
 in the answer as a result.
 
+% DATASRC_DATABASE_DELETEDIFF updated diff table for delete: %1 %2 %3
+Debug message. A difference record for removing a record from the zone is being
+appended to the difference table. The name, type and rdata of the record is
+logged.
+
+% DATASRC_DATABASE_DELETENSEC3 deleted NSEC3 RR: %1 %2
+Debug message. An NSEC3 record is removed from the table. The name, type and
+rdata is logged.
+
+% DATASRC_DATABASE_DELETERR deleted RR: %1 %2 %3
+Debug message. A resource record is removed from the table. The name, type and
+rdata is logged.
+
 % DATASRC_DATABASE_FINDNSEC3 Looking for NSEC3 for %1 in %2 mode
 Debug information. A search in an database data source for NSEC3 that
 matches or covers the given name is being started.

src/lib/datasrc/sqlite3_accessor.cc

@@ -104,7 +104,9 @@ const char* const text_statements[NUM_STATEMENTS] = {
     "INSERT INTO records "      // ADD_RECORD
         "(zone_id, name, rname, ttl, rdtype, sigtype, rdata) "
         "VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)",
-    "DELETE FROM records WHERE zone_id=?1 AND name=?2 " // DEL_RECORD
+    // DEL_RECORD:
+    // Delete based on the reverse name, as that one has an index.
+    "DELETE FROM records WHERE zone_id=?1 AND rname=?2 " // DEL_RECORD
         "AND rdtype=?3 AND rdata=?4",
 
     // ITERATE_RECORDS:
@@ -1295,8 +1297,16 @@ SQLite3Accessor::deleteRecordInZone(const string (&params)[DEL_PARAM_COUNT]) {
         isc_throw(DataSourceError, "deleting record in SQLite3 "
                   "data source without transaction");
     }
-    doUpdate<const string (&)[DEL_PARAM_COUNT]>(
-        *dbparameters_, DEL_RECORD, params, "delete record from zone");
+    // We don't pass all the parameters to the query, one name (reserve one
+    // in this case) is sufficient. Pass only the needed ones.
+    const size_t SQLITE3_DEL_PARAM_COUNT = DEL_PARAM_COUNT - 1;
+    const string sqlite3_params[SQLITE3_DEL_PARAM_COUNT] = {
+        params[DEL_RNAME],
+        params[DEL_TYPE],
+        params[DEL_RDATA]
+    };
+    doUpdate<const string (&)[SQLITE3_DEL_PARAM_COUNT]>(
+        *dbparameters_, DEL_RECORD, sqlite3_params, "delete record from zone");
 }
 
 void
@@ -1307,8 +1317,14 @@ SQLite3Accessor::deleteNSEC3RecordInZone(
         isc_throw(DataSourceError, "deleting NSEC3-related record in SQLite3 "
                   "data source without transaction");
     }
-    doUpdate<const string (&)[DEL_PARAM_COUNT]>(
-        *dbparameters_, DEL_NSEC3_RECORD, params,
+    const size_t SQLITE3_DEL_PARAM_COUNT = DEL_PARAM_COUNT - 1;
+    const string sqlite3_params[SQLITE3_DEL_PARAM_COUNT] = {
+        params[DEL_NAME],
+        params[DEL_TYPE],
+        params[DEL_RDATA]
+    };
+    doUpdate<const string (&)[SQLITE3_DEL_PARAM_COUNT]>(
+        *dbparameters_, DEL_NSEC3_RECORD, sqlite3_params,
         "delete NSEC3 record from zone");
 }
 

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -806,7 +806,7 @@ const char* const new_data[] = {
 };
 const char* const deleted_data[] = {
     // Existing data to be removed commonly used by some of the tests below
-    "foo.bar.example.com.", "A", "192.0.2.1"
+    "foo.bar.example.com.", "A", "192.0.2.1", "com.example.bar.foo."
 };
 const char* const nsec3_data[DatabaseAccessor::ADD_NSEC3_COLUMN_COUNT] = {
     // example NSEC3 parameters.  Using "apex_hash" just as a convenient
@@ -823,7 +823,8 @@ const char* const nsec3_sig_data[DatabaseAccessor::ADD_NSEC3_COLUMN_COUNT] = {
 const char* const nsec3_deleted_data[] = {
     // Delete parameters for nsec3_data
     apex_hash, nsec3_data[DatabaseAccessor::ADD_NSEC3_TYPE],
-    nsec3_data[DatabaseAccessor::ADD_NSEC3_RDATA]
+    nsec3_data[DatabaseAccessor::ADD_NSEC3_RDATA],
+    apex_hash
 };
 
 class SQLite3Update : public SQLite3AccessorTest {
@@ -1222,6 +1223,7 @@ TEST_F(SQLite3Update, deleteNonexistent) {
     // Replace the name with a non existent one, then try to delete it.
     // nothing should happen.
     del_params[DatabaseAccessor::DEL_NAME] = "no-such-name.example.com.";
+    del_params[DatabaseAccessor::DEL_RNAME] = "com.example.no-such-name.";
     checkRecords(*accessor, zone_id, "no-such-name.example.com.",
                  empty_stored);
     accessor->deleteRecordInZone(del_params);
@@ -1535,7 +1537,7 @@ TEST_F(SQLite3Update, addDiffWithUpdate) {
     // the basic tests so far pass.  But we check it in case we miss something.
 
     const char* const old_a_record[] = {
-        "dns01.example.com.", "A", "192.0.2.1"
+        "dns01.example.com.", "A", "192.0.2.1", "com.example.dns01."
     };
     const char* const new_a_record[] = {
         "dns01.example.com.", "com.example.dns01.", "3600", "A", "",
@@ -1544,6 +1546,7 @@ TEST_F(SQLite3Update, addDiffWithUpdate) {
     const char* const old_soa_record[] = {
         "example.com.", "SOA",
         "ns.example.com. admin.example.com. 1234 3600 1800 2419200 7200",
+        "com.example."
     };
     const char* const new_soa_record[] = {
         "dns01.example.com.", "com.example.dns01.", "3600", "A", "",