lib/rplan: removed long-lived wr transactions

the reason is that LMDB allows only 1 write transaction
per thread, and this doesn’t work well with multiplexed
requests. there is probably going to be some performance hit,
but now transactions are scoped for each cache operation

diff --git a/lib/layer/itercache.c b/lib/layer/itercache.c
index 30ef1e06f585886474f04bd1f88e72d8a353c3ad..78365874cc96a548428e4b5994a0a2cdf3b5e140 100644 (file)
--- a/lib/layer/itercache.c
+++ b/lib/layer/itercache.c
@@ -82,7 +82,11 @@ static int read_cache(knot_layer_t *ctx, knot_pkt_t *pkt)
                return ctx->state;
        }
 
-       namedb_txn_t *txn = kr_rplan_txn_acquire(rplan, NAMEDB_RDONLY);
+       namedb_txn_t txn;
+       struct kr_cache *cache = req->ctx->cache;
+       if (kr_cache_txn_begin(cache, &txn, NAMEDB_RDONLY) != 0) {
+               return KNOT_STATE_CONSUME;
+       }
        uint32_t timestamp = cur->timestamp.tv_sec;
        knot_rrset_t cache_rr;
        knot_rrset_init(&cache_rr, cur->sname, cur->stype, cur->sclass);
@@ -94,29 +98,33 @@ static int read_cache(knot_layer_t *ctx, knot_pkt_t *pkt)
        }
 
        /* Try to find expected record first. */
-       int state = read_cache_rr(txn, &cache_rr, timestamp, callback, req);
+       int state = read_cache_rr(&txn, &cache_rr, timestamp, callback, req);
        if (state == KNOT_STATE_DONE) {
                DEBUG_MSG("=> satisfied from cache\n");
                cur->flags |= QUERY_RESOLVED;
+               kr_cache_txn_abort(&txn);
                return state;
        }
 
        /* Check if CNAME chain exists. */
        cache_rr.type = KNOT_RRTYPE_CNAME;
-       state = read_cache_rr(txn, &cache_rr, timestamp, callback, req);
+       state = read_cache_rr(&txn, &cache_rr, timestamp, callback, req);
        if (state != KNOT_STATE_NOOP) {
                if (cur->stype != KNOT_RRTYPE_CNAME) {
                        const knot_dname_t *cname = knot_cname_name(&cache_rr.rrs);
                        if (kr_rplan_push(rplan, cur->parent, cname, cur->sclass, cur->stype) == NULL) {
+                               kr_cache_txn_abort(&txn);
                                return KNOT_STATE_FAIL;
                        }
                }
 
                cur->flags |= QUERY_RESOLVED;
+               kr_cache_txn_abort(&txn);
                return KNOT_STATE_DONE;
        }
 
        /* Not resolved. */
+       kr_cache_txn_abort(&txn);
        return KNOT_STATE_CONSUME;
 }
 
@@ -239,32 +247,35 @@ static int write_cache(knot_layer_t *ctx, knot_pkt_t *pkt)
                return ctx->state;
        }
 
-       /* Open write transaction */
-       mm_ctx_t *pool = rplan->pool;
-       uint32_t timestamp = query->timestamp.tv_sec;
-       namedb_txn_t *txn = kr_rplan_txn_acquire(rplan, 0);
-       if (txn == NULL) {
-               return ctx->state; /* Couldn't acquire cache, ignore. */
-       }
-
        /* Cache only positive answers. */
        /** \todo Negative answers cache support */
        if (knot_wire_get_rcode(pkt->wire) != KNOT_RCODE_NOERROR) {
                return ctx->state;
        }
 
+       /* Open write transaction */
+       mm_ctx_t *pool = rplan->pool;
+       uint32_t timestamp = query->timestamp.tv_sec;
+       struct kr_cache *cache = req->ctx->cache;
+       namedb_txn_t txn;
+       if (kr_cache_txn_begin(cache, &txn, 0) != 0) {
+               return ctx->state; /* Couldn't acquire cache, ignore. */
+       }
+
        /* If authoritative, cache answer for current query. */
        int ret = KNOT_EOK;
        if (knot_wire_get_aa(pkt->wire)) {
-               ret = write_cache_answer(pkt, txn, pool, timestamp);
+               ret = write_cache_answer(pkt, &txn, pool, timestamp);
        }
        if (ret == KNOT_EOK) {
-               ret = write_cache_authority(pkt, txn, pool, timestamp);
+               ret = write_cache_authority(pkt, &txn, pool, timestamp);
        }
 
        /* Cache full, do what we must. */
        if (ret == KNOT_ESPACE) {
-               kr_cache_clear(txn);
+               kr_cache_clear(&txn);
+       } else {
+               kr_cache_txn_commit(&txn);
        }
 
        return ctx->state;

diff --git a/lib/resolve.c b/lib/resolve.c
index 5c4c70e88b638a7690f7516362c13c3b2d770384..faefb2092fdc0523f39dd503278650c4d02d357c 100644 (file)
--- a/lib/resolve.c
+++ b/lib/resolve.c
@@ -335,14 +335,12 @@ int kr_resolve_produce(struct kr_request *request, struct sockaddr **dst, int *t
 
 int kr_resolve_finish(struct kr_request *request, int state)
 {
+#ifndef NDEBUG
        struct kr_rplan *rplan = &request->rplan;
        DEBUG_MSG("finished: %d, mempool: %zu B\n", state, (size_t) mp_total_size(request->pool.ctx));
-
-       /* Resolution success, commit cache transaction. */
-       if (state == KNOT_STATE_DONE) {
-               kr_rplan_txn_commit(rplan);
-       } else {
-               /* Error during procesing, internal failure */
+#endif
+       /* Error during procesing, internal failure */
+       if (state != KNOT_STATE_DONE) {
                knot_pkt_t *answer = request->answer;
                if (knot_wire_get_rcode(answer->wire) == KNOT_RCODE_NOERROR) {
                        knot_wire_set_rcode(answer->wire, KNOT_RCODE_SERVFAIL);

diff --git a/lib/rplan.c b/lib/rplan.c
index 7438b66f1ec1933281ef07b8b57daa06647affb8..c02e61a4f8ffb05ca21501cd9a105fd0d2ffcdc7 100644 (file)
--- a/lib/rplan.c
+++ b/lib/rplan.c
@@ -84,11 +84,6 @@ void kr_rplan_deinit(struct kr_rplan *rplan)
        WALK_LIST_DELSAFE(qry, next, rplan->resolved) {
                query_free(rplan->pool, qry);
        }
-
-       /* Abort any pending transactions. */
-       if (rplan->txn.db != NULL) {
-               kr_cache_txn_abort(&rplan->txn);
-       }
 }
 
 bool kr_rplan_empty(struct kr_rplan *rplan)
@@ -119,9 +114,14 @@ struct kr_query *kr_rplan_push(struct kr_rplan *rplan, struct kr_query *parent,
        add_tail(&rplan->pending, &qry->node);
 
        /* Find closest zone cut for this query. */
-       namedb_txn_t *txn = kr_rplan_txn_acquire(rplan, NAMEDB_RDONLY);
+       namedb_txn_t txn;
        kr_zonecut_init(&qry->zone_cut, name, rplan->pool);
-       kr_zonecut_find_cached(&qry->zone_cut, txn, qry->timestamp.tv_sec);
+       if (kr_cache_txn_begin(rplan->context->cache, &txn, NAMEDB_RDONLY) != 0) {
+               kr_zonecut_set_sbelt(&qry->zone_cut);
+       } else {
+               kr_zonecut_find_cached(&qry->zone_cut, &txn, qry->timestamp.tv_sec);
+               kr_cache_txn_abort(&txn);
+       }
 
 #ifndef NDEBUG
        char name_str[KNOT_DNAME_MAXLEN], type_str[16];
@@ -152,53 +152,6 @@ struct kr_query *kr_rplan_current(struct kr_rplan *rplan)
        return TAIL(rplan->pending);
 }
 
-namedb_txn_t *kr_rplan_txn_acquire(struct kr_rplan *rplan, unsigned flags)
-{
-       if (rplan == NULL || rplan->context == NULL) {
-               return NULL;
-       }
-
-       /* Discard current transaction if RDONLY, but WR is requested. */
-       if ((rplan->txn_flags & NAMEDB_RDONLY) && !(flags & NAMEDB_RDONLY)) {
-               kr_cache_txn_abort(&rplan->txn);
-               rplan->txn.db = NULL;
-       }
-
-       /* Reuse transaction if exists. */
-       if (rplan->txn.db != NULL) {
-               return &rplan->txn;
-       }
-
-       /* Transaction doesn't exist, start new one. */
-       int ret = kr_cache_txn_begin(rplan->context->cache, &rplan->txn, flags);
-       if (ret != KNOT_EOK) {
-               rplan->txn.db = NULL;
-               return NULL;
-       }
-
-       rplan->txn_flags = flags;
-       return &rplan->txn;
-}
-
-int kr_rplan_txn_commit(struct kr_rplan *rplan)
-{
-       if (rplan == NULL || rplan->context == NULL) {
-               return KNOT_EINVAL;
-       }
-
-       /* Just discard RDONLY transactions. */
-       int ret = KNOT_EOK;
-       if (rplan->txn_flags & NAMEDB_RDONLY) {
-               kr_cache_txn_abort(&rplan->txn);
-       } else {
-               /* Commit write transactions. */
-               ret = kr_cache_txn_commit(&rplan->txn);
-       }
-
-       rplan->txn.db = NULL;
-       return ret;
-}
-
 bool kr_rplan_satisfies(struct kr_query *closure, const knot_dname_t *name, uint16_t cls, uint16_t type)
 {
        while (closure != NULL) {

diff --git a/lib/rplan.h b/lib/rplan.h
index 07aa951fa1d74eca53268afc2643898e984dbd49..ef5786c31c8a7b387f4458b909f704dfc70d06a2 100644 (file)
--- a/lib/rplan.h
+++ b/lib/rplan.h
@@ -58,8 +58,6 @@ struct kr_query {
  * It also keeps a notion of current zone cut.
  */
 struct kr_rplan {
-       unsigned txn_flags;          /**< Current transaction flags. */
-       namedb_txn_t txn;            /**< Current transaction (may be r/o). */
        list_t pending;              /**< List of pending queries. */
        list_t resolved;             /**< List of resolved queries. */
        struct kr_context *context;  /**< Parent resolution context. */
@@ -87,23 +85,6 @@ void kr_rplan_deinit(struct kr_rplan *rplan);
  */
 bool kr_rplan_empty(struct kr_rplan *rplan);
 
-/**
- * Acquire rplan transaction (read or write only).
- * @note The transaction is shared during the whole resolution, read only transactions
- *       may be promoted to write-enabled transactions if requested, but never demoted.
- * @param rplan plan instance
- * @param flags transaction flags
- * @return transaction instance or NULL
- */
-namedb_txn_t *kr_rplan_txn_acquire(struct kr_rplan *rplan, unsigned flags);
-
-/**
- * Commit any existing transaction, read-only transactions may be just aborted.
- * @param rplan plan instance
- * @return KNOT_E*
- */
-int kr_rplan_txn_commit(struct kr_rplan *rplan);
-
 /**
  * Push a query to the top of the resolution plan.
  * @note This means that this query takes precedence before all pending queries.

diff --git a/tests/test_rplan.c b/tests/test_rplan.c
index f25bc9ba1c66912f50c2faeee53bfeb95e05925c..8a8d5d6496eea186f13bab234b7c28bb4e7791f2 100644 (file)
--- a/tests/test_rplan.c
+++ b/tests/test_rplan.c
@@ -27,8 +27,6 @@ static void test_rplan_params(void **state)
        assert_int_equal(kr_rplan_pop(NULL, NULL), KNOT_EINVAL);
        assert_true(kr_rplan_empty(NULL));
        assert_null(kr_rplan_current(NULL));
-       assert_null(kr_rplan_txn_acquire(NULL, 0));
-       assert_int_equal(kr_rplan_txn_commit(NULL), KNOT_EINVAL);
        kr_rplan_deinit(NULL);
 
        /* NULL mandatory parameters */
@@ -39,8 +37,6 @@ static void test_rplan_params(void **state)
        assert_int_equal(kr_rplan_pop(&rplan, NULL), KNOT_EINVAL);
        assert_true(kr_rplan_empty(&rplan));
        assert_null(kr_rplan_current(&rplan));
-       assert_null(kr_rplan_txn_acquire(&rplan, 0));
-       assert_int_equal(kr_rplan_txn_commit(&rplan), KNOT_EINVAL);
        kr_rplan_deinit(&rplan);
 }
 

diff --git a/tests/testdata/iter_cname_qnamecopy.rpl b/tests/testdata/iter_cname_qnamecopy.rpl
index 12019816ac16d2e2095b586cd9e3e40c2bf620bb..9539d34ced4b1ea6c7e3a368264cd57a77005e7e 100644 (file)
--- a/tests/testdata/iter_cname_qnamecopy.rpl
+++ b/tests/testdata/iter_cname_qnamecopy.rpl
@@ -171,7 +171,7 @@ www.example.com. IN A
 SECTION ANSWER
 www.example.com. IN CNAME      www.next.com.
 SECTION AUTHORITY
-next.com.      IN SOA next.com. next.com. 2007090400 28800 7200 604800 18000
+;next.com.     IN SOA next.com. next.com. 2007090400 28800 7200 604800 18000
 SECTION ADDITIONAL
 ENTRY_END