add setup/commit functions to rdatacallbacks

because dns_qpmulti_commit() can be time consuming, it's inefficient
to open and commit a qpmulti transaction for each rdataset being loaded
into a database.  we can improve load time by opening a qpmulti
transaction before adding a group of rdatasets and then committing it
afterward.

this commit adds 'setup' and 'commit' functions to dns_rdatacallbacks_t,
which can be called before and after the loops in which 'add' is
called in dns_master_load() and axfr_apply().

diff --git a/lib/dns/callbacks.c b/lib/dns/callbacks.c
index 34ad8fdf92484b2d568b9d88bde7b32dc6573102..e034155efc97532c7417144aa6b8357a1124076e 100644 (file)
--- a/lib/dns/callbacks.c
+++ b/lib/dns/callbacks.c
@@ -78,13 +78,9 @@ static void
 dns_rdatacallbacks_initcommon(dns_rdatacallbacks_t *callbacks) {
        REQUIRE(callbacks != NULL);
 
-       callbacks->magic = DNS_CALLBACK_MAGIC;
-       callbacks->add = NULL;
-       callbacks->rawdata = NULL;
-       callbacks->zone = NULL;
-       callbacks->add_private = NULL;
-       callbacks->error_private = NULL;
-       callbacks->warn_private = NULL;
+       *callbacks = (dns_rdatacallbacks_t){
+               .magic = DNS_CALLBACK_MAGIC,
+       };
 }
 
 /*

diff --git a/lib/dns/diff.c b/lib/dns/diff.c
index afbf87c5e8d752ed823f985af8244d119a880298..efefbaa2a0bddde289f8c9617ed37b687585a84b 100644 (file)
--- a/lib/dns/diff.c
+++ b/lib/dns/diff.c
@@ -24,6 +24,7 @@
 #include <isc/string.h>
 #include <isc/util.h>
 
+#include <dns/callbacks.h>
 #include <dns/db.h>
 #include <dns/diff.h>
 #include <dns/log.h>
@@ -506,13 +507,16 @@ dns_diff_applysilently(dns_diff_t *diff, dns_db_t *db, dns_dbversion_t *ver) {
 /* XXX this duplicates lots of code in diff_apply(). */
 
 isc_result_t
-dns_diff_load(dns_diff_t *diff, dns_addrdatasetfunc_t addfunc,
-             void *add_private) {
+dns_diff_load(dns_diff_t *diff, dns_rdatacallbacks_t *callbacks) {
        dns_difftuple_t *t;
        isc_result_t result;
 
        REQUIRE(DNS_DIFF_VALID(diff));
 
+       if (callbacks->setup != NULL) {
+               callbacks->setup(callbacks->add_private);
+       }
+
        t = ISC_LIST_HEAD(diff->tuples);
        while (t != NULL) {
                dns_name_t *name;
@@ -551,8 +555,8 @@ dns_diff_load(dns_diff_t *diff, dns_addrdatasetfunc_t addfunc,
                        rds.trust = dns_trust_ultimate;
 
                        INSIST(op == DNS_DIFFOP_ADD);
-                       result = (*addfunc)(add_private, name,
-                                           &rds DNS__DB_FILELINE);
+                       result = callbacks->add(callbacks->add_private, name,
+                                               &rds DNS__DB_FILELINE);
                        if (result == DNS_R_UNCHANGED) {
                                isc_log_write(DIFF_COMMON_LOGARGS,
                                              ISC_LOG_WARNING,
@@ -570,7 +574,11 @@ dns_diff_load(dns_diff_t *diff, dns_addrdatasetfunc_t addfunc,
                }
        }
        result = ISC_R_SUCCESS;
+
 failure:
+       if (callbacks->commit != NULL) {
+               callbacks->commit(callbacks->add_private);
+       }
        return (result);
 }
 

diff --git a/lib/dns/include/dns/callbacks.h b/lib/dns/include/dns/callbacks.h
index e908a9f04886913b07336a3ba67a1b77621b0b7a..da15ec9255e87e4273f6a16663bfc41ff1d97751 100644 (file)
--- a/lib/dns/include/dns/callbacks.h
+++ b/lib/dns/include/dns/callbacks.h
@@ -37,9 +37,20 @@ struct dns_rdatacallbacks {
        unsigned int magic;
 
        /*%
-        * dns_load_master calls this when it has rdatasets to commit.
+        * dns_load_master calls 'add' when it has an rdataset to add
+        * to the database. If defined, it calls 'setup' before and
+        * 'commit' after adding rdatasets.
+        *
+        * Some database implementations will commit each rdataset as
+        * soon as it's added, in which case 'setup' and 'commit' need
+        * not be defined.  However, other implementations can be
+        * optimized by grouping rdatasets into a transaction; the
+        * setup and commit functions allow this transaction to be
+        * opened and committed.
         */
        dns_addrdatasetfunc_t add;
+       dns_transactionfunc_t setup;
+       dns_transactionfunc_t commit;
 
        /*%
         * dns_master_load*() call this when loading a raw zonefile,

diff --git a/lib/dns/include/dns/diff.h b/lib/dns/include/dns/diff.h
index 841e796004abefcd1dd728614c601521b4efd27b..3bda5644d6eb7a9c1135f251265f20810076d691 100644 (file)
--- a/lib/dns/include/dns/diff.h
+++ b/lib/dns/include/dns/diff.h
@@ -243,19 +243,15 @@ dns_diff_applysilently(dns_diff_t *diff, dns_db_t *db, dns_dbversion_t *ver);
  */
 
 isc_result_t
-dns_diff_load(dns_diff_t *diff, dns_addrdatasetfunc_t addfunc,
-             void *add_private);
+dns_diff_load(dns_diff_t *diff, dns_rdatacallbacks_t *callbacks);
 /*%<
  * Like dns_diff_apply, but for use when loading a new database
  * instead of modifying an existing one.  This bypasses the
  * database transaction mechanisms.
  *
  * Requires:
- *\li  'addfunc' is a valid dns_addradatasetfunc_t obtained from
- *     dns_db_beginload()
- *
- *\li  'add_private' points to a corresponding dns_dbload_t *
- *      (XXX why is it a void pointer, then?)
+ *\li  'callbacks' points to a dns_rdatacallbacks_t structure obtained
+ *     from dns_db_beginload()
  */
 
 isc_result_t

diff --git a/lib/dns/include/dns/types.h b/lib/dns/include/dns/types.h
index b938d13752bb7a476ec8e6817191017062cdd4c2..852ed865667fa74edf6e8869ca6db60d8dda6a41 100644 (file)
--- a/lib/dns/include/dns/types.h
+++ b/lib/dns/include/dns/types.h
@@ -436,6 +436,7 @@ typedef void (*dns_rawdatafunc_t)(dns_zone_t *, dns_masterrawheader_t *);
 typedef isc_result_t (*dns_addrdatasetfunc_t)(void *arg, const dns_name_t *name,
                                              dns_rdataset_t *rdataset
                                                      DNS__DB_FLARG);
+typedef void (*dns_transactionfunc_t)(void *arg);
 
 typedef isc_result_t (*dns_additionaldatafunc_t)(
        void *arg, const dns_name_t *name, dns_rdatatype_t type,

diff --git a/lib/dns/master.c b/lib/dns/master.c
index 81cc999b00b6c8602cc6a75e51ea371165a0cc35..a9e7960d9633bc5617591b96f0432c91550cf9a9 100644 (file)
--- a/lib/dns/master.c
+++ b/lib/dns/master.c
@@ -1007,7 +1007,7 @@ load_text(dns_loadctx_t *lctx) {
        dns_rdataclass_t rdclass;
        dns_rdatatype_t type, covers;
        uint32_t ttl_offset = 0;
-       dns_name_t *new_name;
+       dns_name_t *new_name = NULL;
        bool current_has_delegation = false;
        bool finish_origin = false;
        bool finish_include = false;
@@ -1018,9 +1018,9 @@ load_text(dns_loadctx_t *lctx) {
        isc_result_t result = ISC_R_UNEXPECTED;
        rdatalist_head_t glue_list;
        rdatalist_head_t current_list;
-       dns_rdatalist_t *this;
+       dns_rdatalist_t *this = NULL;
        dns_rdatalist_t *rdatalist = NULL;
-       dns_rdatalist_t *new_rdatalist;
+       dns_rdatalist_t *new_rdatalist = NULL;
        int rdlcount = 0;
        int rdlcount_save = 0;
        int rdatalist_size = 0;
@@ -1029,21 +1029,21 @@ load_text(dns_loadctx_t *lctx) {
        isc_buffer_t target_ft;
        isc_buffer_t target_save;
        dns_rdata_t *rdata = NULL;
-       dns_rdata_t *new_rdata;
+       dns_rdata_t *new_rdata = NULL;
        int rdcount = 0;
        int rdcount_save = 0;
        int rdata_size = 0;
        unsigned char *target_mem = NULL;
        int target_size = TSIZ;
        int new_in_use;
-       isc_mem_t *mctx;
-       dns_rdatacallbacks_t *callbacks;
-       dns_incctx_t *ictx;
+       isc_mem_t *mctx = NULL;
+       dns_rdatacallbacks_t *callbacks = NULL;
+       dns_incctx_t *ictx = NULL;
        char *range = NULL;
        char *lhs = NULL;
        char *gtype = NULL;
        char *rhs = NULL;
-       const char *source;
+       const char *source = NULL;
        unsigned long line = 0;
        bool explicit_ttl;
        char classname1[DNS_RDATACLASS_FORMATSIZE];
@@ -1066,6 +1066,11 @@ load_text(dns_loadctx_t *lctx) {
        isc_buffer_init(&target, target_mem, target_size);
        target_save = target;
 
+       /* open a database transaction */
+       if (callbacks->setup != NULL) {
+               callbacks->setup(callbacks->add_private);
+       }
+
        if ((lctx->options & DNS_MASTER_CHECKNAMES) != 0) {
                options |= DNS_RDATA_CHECKNAMES;
        }
@@ -2116,6 +2121,11 @@ insist_and_cleanup:
        INSIST(result != ISC_R_SUCCESS);
 
 cleanup:
+       /* commit the database transaction */
+       if (callbacks->commit != NULL) {
+               callbacks->commit(callbacks->add_private);
+       }
+
        while ((this = ISC_LIST_HEAD(current_list)) != NULL) {
                ISC_LIST_UNLINK(current_list, this, link);
        }
@@ -2357,6 +2367,11 @@ load_raw(dns_loadctx_t *lctx) {
 
        name = dns_fixedname_initname(&fixed);
 
+       /* open a database transaction */
+       if (callbacks->setup != NULL) {
+               callbacks->setup(callbacks->add_private);
+       }
+
        /*
         * In the following loop, we regard any error fatal regardless of
         * whether "MANYERRORS" is set in the context option.  This is because
@@ -2598,6 +2613,11 @@ load_raw(dns_loadctx_t *lctx) {
        }
 
 cleanup:
+       /* commit the database transaction */
+       if (callbacks->commit != NULL) {
+               callbacks->commit(callbacks->add_private);
+       }
+
        if (rdata != NULL) {
                isc_mem_cput(mctx, rdata, rdata_size, sizeof(*rdata));
        }
@@ -2888,17 +2908,14 @@ commit(dns_rdatacallbacks_t *callbacks, dns_loadctx_t *lctx,
        unsigned int line) {
        dns_rdatalist_t *this;
        dns_rdataset_t dataset;
-       isc_result_t result;
+       isc_result_t result = ISC_R_SUCCESS;
        char namebuf[DNS_NAME_FORMATSIZE];
        void (*error)(struct dns_rdatacallbacks *, const char *, ...);
 
        this = ISC_LIST_HEAD(*head);
        error = callbacks->error;
 
-       if (this == NULL) {
-               return (ISC_R_SUCCESS);
-       }
-       do {
+       while (this != NULL) {
                dns_rdataset_init(&dataset);
                dns_rdatalist_tordataset(this, &dataset);
                dataset.trust = dns_trust_ultimate;
@@ -2911,8 +2928,8 @@ commit(dns_rdatacallbacks_t *callbacks, dns_loadctx_t *lctx,
                        dataset.attributes |= DNS_RDATASETATTR_RESIGN;
                        dataset.resign = resign_fromlist(this, lctx);
                }
-               result = ((*callbacks->add)(callbacks->add_private, owner,
-                                           &dataset DNS__DB_FILELINE));
+               result = callbacks->add(callbacks->add_private, owner,
+                                       &dataset DNS__DB_FILELINE);
                if (result == ISC_R_NOMEMORY) {
                        (*error)(callbacks, "dns_master_load: %s",
                                 isc_result_totext(result));
@@ -2931,12 +2948,13 @@ commit(dns_rdatacallbacks_t *callbacks, dns_loadctx_t *lctx,
                if (MANYERRS(lctx, result)) {
                        SETRESULT(lctx, result);
                } else if (result != ISC_R_SUCCESS) {
-                       return (result);
+                       break;
                }
                ISC_LIST_UNLINK(*head, this, link);
                this = ISC_LIST_HEAD(*head);
-       } while (this != NULL);
-       return (ISC_R_SUCCESS);
+       }
+
+       return (result);
 }
 
 /*

diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c
index 1ec6690a11a99ea112bbe2f46356722492040238..bf0d199208862bb7a76f94d1d870abe690031caf 100644 (file)
--- a/lib/dns/qpzone.c
+++ b/lib/dns/qpzone.c
@@ -1682,32 +1682,30 @@ delegating_type(qpzonedb_t *qpdb, qpdata_t *node, dns_typepair_t type) {
 }
 
 static void
-loading_addnode(qpzonedb_t *qpdb, const dns_name_t *name, dns_rdatatype_t type,
-               dns_rdatatype_t covers, qpdata_t **nodep) {
+loading_addnode(qpdb_load_t *loadctx, const dns_name_t *name,
+               dns_rdatatype_t type, dns_rdatatype_t covers,
+               qpdata_t **nodep) {
+       qpzonedb_t *qpdb = (qpzonedb_t *)loadctx->db;
        isc_result_t result;
        qpdata_t *node = NULL, *nsecnode = NULL;
-       dns_qp_t *qp = NULL, *nsec = NULL;
 
        if (type == dns_rdatatype_nsec3 || covers == dns_rdatatype_nsec3) {
-               dns_qpmulti_write(qpdb->nsec3, &qp);
-               result = dns_qp_getname(qp, name, (void **)&node, NULL);
+               result = dns_qp_getname(loadctx->nsec3, name, (void **)&node,
+                                       NULL);
                if (result == ISC_R_SUCCESS) {
                        *nodep = node;
                } else {
                        node = new_qpdata(qpdb, name);
-                       result = dns_qp_insert(qp, node, 0);
+                       result = dns_qp_insert(loadctx->nsec3, node, 0);
                        INSIST(result == ISC_R_SUCCESS);
                        node->nsec = DNS_DB_NSEC_NSEC3;
                        *nodep = node;
                        qpdata_detach(&node);
                }
-               dns_qp_compact(qp, DNS_QPGC_MAYBE);
-               dns_qpmulti_commit(qpdb->nsec3, &qp);
                return;
        }
 
-       dns_qpmulti_write(qpdb->tree, &qp);
-       result = dns_qp_getname(qp, name, (void **)&node, NULL);
+       result = dns_qp_getname(loadctx->tree, name, (void **)&node, NULL);
        if (result == ISC_R_SUCCESS) {
                if (type == dns_rdatatype_nsec &&
                    node->nsec == DNS_DB_NSEC_HAS_NSEC)
@@ -1717,7 +1715,7 @@ loading_addnode(qpzonedb_t *qpdb, const dns_name_t *name, dns_rdatatype_t type,
        } else {
                INSIST(node == NULL);
                node = new_qpdata(qpdb, name);
-               result = dns_qp_insert(qp, node, 0);
+               result = dns_qp_insert(loadctx->tree, node, 0);
                INSIST(result == ISC_R_SUCCESS);
                qpdata_unref(node);
        }
@@ -1730,9 +1728,8 @@ loading_addnode(qpzonedb_t *qpdb, const dns_name_t *name, dns_rdatatype_t type,
         * too. This tree speeds searches for closest NSECs that would
         * otherwise need to examine many irrelevant nodes in large TLDs.
         */
-       dns_qpmulti_write(qpdb->nsec, &nsec);
        nsecnode = new_qpdata(qpdb, name);
-       result = dns_qp_insert(nsec, nsecnode, 0);
+       result = dns_qp_insert(loadctx->nsec, nsecnode, 0);
        node->nsec = DNS_DB_NSEC_HAS_NSEC;
        if (result == ISC_R_SUCCESS) {
                nsecnode->nsec = DNS_DB_NSEC_NSEC;
@@ -1740,13 +1737,6 @@ loading_addnode(qpzonedb_t *qpdb, const dns_name_t *name, dns_rdatatype_t type,
        qpdata_detach(&nsecnode);
 
 done:
-       if (nsec != NULL) {
-               dns_qp_compact(nsec, DNS_QPGC_MAYBE);
-               dns_qpmulti_commit(qpdb->nsec, &nsec);
-       }
-
-       dns_qp_compact(qp, DNS_QPGC_MAYBE);
-       dns_qpmulti_commit(qpdb->tree, &qp);
        *nodep = node;
 }
 
@@ -2167,7 +2157,6 @@ loading_addrdataset(void *arg, const dns_name_t *name,
        isc_region_t region;
        dns_slabheader_t *newheader = NULL;
        isc_rwlocktype_t nlocktype = isc_rwlocktype_none;
-       dns_qp_t *qp = NULL;
 
        REQUIRE(rdataset->rdclass == qpdb->common.rdclass);
 
@@ -2180,11 +2169,10 @@ loading_addrdataset(void *arg, const dns_name_t *name,
                return (DNS_R_NOTZONETOP);
        }
 
-       dns_qpmulti_write(qpdb->tree, &qp);
        if (rdataset->type != dns_rdatatype_nsec3 &&
            rdataset->covers != dns_rdatatype_nsec3)
        {
-               addwildcards(qpdb, qp, name, false);
+               addwildcards(qpdb, loadctx->tree, name, false);
        }
 
        if (dns_name_iswildcard(name)) {
@@ -2192,23 +2180,20 @@ loading_addrdataset(void *arg, const dns_name_t *name,
                        /*
                         * NS owners cannot legally be wild cards.
                         */
-                       result = DNS_R_INVALIDNS;
-               } else if (rdataset->type == dns_rdatatype_nsec3) {
+                       return (DNS_R_INVALIDNS);
+               }
+
+               if (rdataset->type == dns_rdatatype_nsec3) {
                        /*
                         * NSEC3 owners cannot legally be wild cards.
                         */
-                       result = DNS_R_INVALIDNSEC3;
-               } else {
-                       wildcardmagic(qpdb, qp, name, false);
+                       return (DNS_R_INVALIDNSEC3);
                }
-       }
-       dns_qp_compact(qp, DNS_QPGC_MAYBE);
-       dns_qpmulti_commit(qpdb->tree, &qp);
-       if (result != ISC_R_SUCCESS) {
-               return (result);
+
+               wildcardmagic(qpdb, loadctx->tree, name, false);
        }
 
-       loading_addnode(qpdb, name, rdataset->type, rdataset->covers, &node);
+       loading_addnode(loadctx, name, rdataset->type, rdataset->covers, &node);
        result = dns_rdataslab_fromrdataset(rdataset, qpdb->common.mctx,
                                            &region, sizeof(dns_slabheader_t));
        if (result != ISC_R_SUCCESS) {
@@ -2252,6 +2237,35 @@ loading_addrdataset(void *arg, const dns_name_t *name,
        return (result);
 }
 
+static void
+loading_setup(void *arg) {
+       qpdb_load_t *loadctx = arg;
+       qpzonedb_t *qpdb = (qpzonedb_t *)loadctx->db;
+
+       dns_qpmulti_write(qpdb->tree, &loadctx->tree);
+       dns_qpmulti_write(qpdb->nsec, &loadctx->nsec);
+       dns_qpmulti_write(qpdb->nsec3, &loadctx->nsec3);
+}
+
+static void
+loading_commit(void *arg) {
+       qpdb_load_t *loadctx = arg;
+       qpzonedb_t *qpdb = (qpzonedb_t *)loadctx->db;
+
+       if (loadctx->tree != NULL) {
+               dns_qp_compact(loadctx->tree, DNS_QPGC_MAYBE);
+               dns_qpmulti_commit(qpdb->tree, &loadctx->tree);
+       }
+       if (loadctx->nsec != NULL) {
+               dns_qp_compact(loadctx->nsec, DNS_QPGC_MAYBE);
+               dns_qpmulti_commit(qpdb->nsec, &loadctx->nsec);
+       }
+       if (loadctx->nsec3 != NULL) {
+               dns_qp_compact(loadctx->nsec3, DNS_QPGC_MAYBE);
+               dns_qpmulti_commit(qpdb->nsec3, &loadctx->nsec3);
+       }
+}
+
 static isc_result_t
 beginload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) {
        qpdb_load_t *loadctx = NULL;
@@ -2262,9 +2276,7 @@ beginload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) {
        REQUIRE(VALID_QPZONE(qpdb));
 
        loadctx = isc_mem_get(qpdb->common.mctx, sizeof(*loadctx));
-
-       loadctx->db = db;
-       loadctx->now = 0;
+       *loadctx = (qpdb_load_t){ .db = db };
 
        RWLOCK(&qpdb->lock, isc_rwlocktype_write);
 
@@ -2275,6 +2287,8 @@ beginload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) {
        RWUNLOCK(&qpdb->lock, isc_rwlocktype_write);
 
        callbacks->add = loading_addrdataset;
+       callbacks->setup = loading_setup;
+       callbacks->commit = loading_commit;
        callbacks->add_private = loadctx;
 
        return (ISC_R_SUCCESS);
@@ -2308,6 +2322,8 @@ endload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) {
        }
 
        callbacks->add = NULL;
+       callbacks->setup = NULL;
+       callbacks->commit = NULL;
        callbacks->add_private = NULL;
 
        isc_mem_put(qpdb->common.mctx, loadctx, sizeof(*loadctx));

diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c
index 2c6e7b0f2ce2fb9c10fc8206e6a7eb29fa12d880..0edf52497f3bb296aa68f89d5e4a544781942eac 100644 (file)
--- a/lib/dns/xfrin.c
+++ b/lib/dns/xfrin.c
@@ -343,7 +343,7 @@ axfr_apply(void *arg) {
                goto failure;
        }
 
-       CHECK(dns_diff_load(&xfr->diff, xfr->axfr.add, xfr->axfr.add_private));
+       CHECK(dns_diff_load(&xfr->diff, &xfr->axfr));
        if (xfr->maxrecords != 0U) {
                result = dns_db_getsize(xfr->db, xfr->ver, &records, NULL);
                if (result == ISC_R_SUCCESS && records > xfr->maxrecords) {