Fix, and write tests for, boolean ops.

include/graph.h

@@ -69,7 +69,8 @@ LSUP_graph_get_txn (
  *
  * The destination graph is not initialized here, so the copy is cumulative.
  *
- * @param[in] txn Transaction handle. It may be NULL.
+ * @param[in] txn Transaction handle. It may be NULL, or an open transaction
+ *  handle, in which case the copy is done within the specified transaction.
  *
  * @param src[in] Source graph.
  *
@@ -91,7 +92,8 @@ LSUP_graph_copy_contents_txn (
  * between two other graphs. The resulting graph may be of any store type and
  * may be the result of graphs of different store types.
  *
- * @param[in] txn Transaction handle. It may be NULL.
+ * @param[in] txn R/W transaction handle for the destination store.  It may be
+ * NULL, or an open transaction within which the operation is performed.
  *
  * @param op[in] Operation to perform. One of #LSUP_bool_op.
  *
@@ -101,7 +103,9 @@ LSUP_graph_copy_contents_txn (
  *
  * @param res[out] Result graph. The handle should be initialized via
  *  #LSUP_graph_new() or equivalent. Any preexisting contents are not removed.
- *  If an unrecoverable error occurs, this graph is freed.
+ *  If an unrecoverable error occurs, this graph is freed, and any preexisting
+ *  triples are lost. Therefore, reusing a result graph handle should only be
+ *  done in tightly controlled loops or sequences.
  *
  * @return LSUP_OK on success; <0 on error.
  */

include/store.h

@@ -26,13 +26,12 @@
  * Add new store implementations to this table.
  */
 #define BACKEND_TBL                                                           \
-/*          #enum pfx   #store if       */\
+/*          #enum suffix#store if       */\
     ENTRY(  HTABLE,     htstore_int  )  \
     ENTRY(  MDB,        mdbstore_int ) \
 
 
-/** @brief Store types. All prefixed with `LSUP_STORE_`.
- */
+/// Store types. All prefixed with `LSUP_STORE_`.
 typedef enum {
 #define ENTRY(a, b) \
     LSUP_STORE_##a,

include/store_interface.h

@@ -284,14 +284,14 @@ typedef LSUP_rc (*store_add_term_fn_t)(
  * which case search is done in all contexts. Note that triples inserted
  * without context are assigned the *default* context for the store.
  *
+ *  @param[in] udata User data. Consult individual store implementations for
+ *   how this is interpreted.
+ *
  * @param[out] ct If not NULL, this will be populated with the number of
  *  entries found. It is very inexpensive to set for lookups without context,
  *  much less so for 1-bound and 2-bound context lookups, in which cases it
  *  should be set only if needed.
  *
- *  @param[in] udata User data. Consult individual store implementations for
- *   how this is interpreted.
- *
  * @return Iterator handle that will be populated with a result iterator. This
  * is always created even if no matches are found and must be freed with
  * #LSUP_mdbiter_free() after use. If matches are found, the iterator points to

src/graph.c

@@ -117,6 +117,7 @@ LSUP_graph_bool_op_txn (
     /* BEGIN union block. */
 
     if (op == LSUP_BOOL_UNION) {
+        // No need to use a transaction here: the graph is freed on failure.
         rc = LSUP_graph_copy_contents (gr1, res);
         PCHECK (rc, fail);
         rc = LSUP_graph_copy_contents (gr2, res);
@@ -127,8 +128,6 @@ LSUP_graph_bool_op_txn (
 
     /* END union block. */
 
-    /* BEGIN subtraction, intersection, XOR block. */
-
     LSUP_Buffer
         *res_sc = LSUP_term_serialize (res->uri),
         *gr1_sc = LSUP_term_serialize (gr1->uri),
@@ -137,33 +136,65 @@ LSUP_graph_bool_op_txn (
     LSUP_BufferTriple *sspo = BTRP_DUMMY;
     size_t ct;
 
+    // Handle transactions for graphs possibly in different storage back ends.
+    void
+        *lu1_txn = NULL,
+        *lu2_txn = NULL;
+
     add_it = res->store->sif->add_init_fn (res->store->data, res_sc, txn);
 
+    // If either source graph is in the same store as the destination and has
+    // an open transaction, reuse that transaction. A new reader cannot be
+    // opened in LMDB while a writer is open already.
+    if (
+            gr1->store->sif->features & LSUP_STORE_TXN
+            && gr1->store == res->store)
+        lu1_txn = res->store->sif->iter_txn_fn (add_it);
+    if (gr1->store->sif->features & LSUP_STORE_TXN) {
+        if (gr1->store == res->store)
+            lu2_txn = res->store->sif->iter_txn_fn (add_it);
+        else if (gr2->store == gr1->store)
+            lu2_txn = lu1_txn;
+    }
+
+    /* BEGIN XOR block. */
+
     if (op == LSUP_BOOL_XOR) {
         // Add triples from gr2 if not found in gr1.
         lu2_it = gr2->store->sif->lookup_fn (
-                gr2->store->data, NULL, NULL, NULL, gr2_sc, NULL, txn);
+                gr2->store->data, NULL, NULL, NULL, gr2_sc, lu2_txn, NULL);
         while (gr2->store->sif->lu_next_fn (lu2_it, sspo, NULL) == LSUP_OK) {
             lu1_it = gr1->store->sif->lookup_fn (
                     gr1->store->data, sspo->s, sspo->p, sspo->o, gr1_sc,
-                    txn, &ct);
-            if (ct == 0)
+                    lu1_txn, &ct);
+            if (ct == 0) {
                 res->store->sif->add_iter_fn (add_it, sspo);
+                rc = LSUP_OK;
+            }
             gr1->store->sif->lu_free_fn (lu1_it);
         }
         gr2->store->sif->lu_free_fn (lu2_it);
     }
 
+    /* BEGIN subtraction and intersection block. */
+
     lu1_it = gr1->store->sif->lookup_fn (
-            gr1->store->data, NULL, NULL, NULL, gr1_sc, txn, NULL);
+            gr1->store->data, NULL, NULL, NULL, gr1_sc, lu1_txn, NULL);
     while (gr1->store->sif->lu_next_fn (lu1_it, sspo, NULL) == LSUP_OK) {
         lu2_it = gr2->store->sif->lookup_fn (
                 gr2->store->data, sspo->s, sspo->p, sspo->o, gr2_sc,
-                txn, &ct);
+                lu2_txn, &ct);
+        if (UNLIKELY (!lu2_it)) {
+            rc = LSUP_DB_ERR;
+            gr1->store->sif->lu_free_fn (lu1_it);
+            goto fail;
+        }
         // For XOR and subtraction, add if not found.
         // For intersection, add if found.
-        if ((ct == 0) ^ (op == LSUP_BOOL_INTERSECTION))
+        if ((ct == 0) ^ (op == LSUP_BOOL_INTERSECTION)) {
             res->store->sif->add_iter_fn (add_it, sspo);
+            rc = LSUP_OK;
+        }
         gr2->store->sif->lu_free_fn (lu2_it);
     }
     gr1->store->sif->lu_free_fn (lu1_it);
@@ -392,11 +423,6 @@ LSUP_graph_remove_txn (
 }
 
 
-/**
- * Copy triples from a source graph into a destination one.
- *
- * The destination graph is not initialized here, so the copy is cumulative.
- */
 LSUP_rc
 LSUP_graph_copy_contents_txn (
         void *txn, const LSUP_Graph *src, LSUP_Graph *dest)

src/store_mdb.c

@@ -567,7 +567,7 @@ mdbstore_add_init (void *h, const LSUP_Buffer *sc, void *th)
     it->store = store;
     it->i = 0;
 
-    mdb_txn_begin (store->env, (MDB_txn *) th, 0, &it->txn);
+    CHECK (mdb_txn_begin (store->env, (MDB_txn *) th, 0, &it->txn), fail);
 
     if (sc) {
         // Store context if it's not the default one.
@@ -594,6 +594,11 @@ mdbstore_add_init (void *h, const LSUP_Buffer *sc, void *th)
     }
 
     return it;
+
+fail:
+    free (it);
+
+    return NULL;
 }
 
 
@@ -770,6 +775,7 @@ mdbstore_lookup (
             log_error ("Database error: %s", LSUP_strerror (it->rc));
             return NULL;
         }
+        LOG_TRACE ("Opening new MDB transaction @%p", it->txn);
         it->flags |= ITER_OPEN_TXN;
     }
 

test/assets/triples.h

@@ -50,7 +50,7 @@ LSUP_Triple **create_triples()
     trp[7] = LSUP_triple_new (s0, p2, o5);
     // Duplicate of trp[7]. Do not double-free.
     trp[8] = LSUP_triple_new (s0, p2, o5);
-    // Duplicate of trp[7] from different terms but semantically
+    // Duplicate of trp[7] from different reused terms but semantically
     // identical. Do not double-free.
     trp[9] = LSUP_triple_new (s5, p4, o5);
 

test/test_graph.c

@@ -140,6 +140,109 @@ _graph_get (LSUP_StoreType type)
 }
 
 
+static int
+_graph_bool_ops (LSUP_StoreType type)
+{
+    const LSUP_StoreInt *sif = LSUP_store_int (type);
+    // Skip if the store doesn't support contexts.
+    if (!(sif->features & LSUP_STORE_CTX)) return 0;
+
+    if (sif->setup_fn) sif->setup_fn (NULL, true);
+
+    LSUP_Triple **trp = create_triples();
+
+    LSUP_Store *store = LSUP_store_new (type, NULL, 0);
+    LSUP_Graph
+        *gr1 = LSUP_graph_new (store, NULL, NULL),
+        *gr2 = LSUP_graph_new (store, NULL, NULL),
+        *gr_dest;
+
+    // Add 2 groups of triples to different graphs.
+    void *it;
+
+    it = LSUP_graph_add_init (gr1);
+    for (size_t i = 0; i < 4; i++) {
+        LSUP_graph_add_iter(it, trp[i]);
+    }
+    LSUP_graph_add_done (it);
+
+    // Skip duplicate triples, we already tested those.
+    // trp[3] is in both graphs.
+    it = LSUP_graph_add_init (gr2);
+    for (size_t i = 3; i < 8; i++) {
+        LSUP_graph_add_iter(it, trp[i]);
+    }
+    LSUP_graph_add_done (it);
+
+    // Test union.
+    gr_dest = LSUP_graph_new (store, NULL, NULL);
+    EXPECT_PASS (LSUP_graph_bool_op (LSUP_BOOL_UNION, gr1, gr2, gr_dest));
+    for (size_t i = 0; i < 8; i++)
+        ASSERT (LSUP_graph_contains (gr_dest, trp[i]), "Union test failed!");
+    LSUP_graph_free (gr_dest);
+
+    // Test subtraction.
+    gr_dest = LSUP_graph_new (store, NULL, NULL);
+    EXPECT_PASS (LSUP_graph_bool_op (
+                LSUP_BOOL_SUBTRACTION, gr1, gr2, gr_dest));
+    for (size_t i = 0; i < 3; i++)
+        ASSERT (LSUP_graph_contains (
+                    gr_dest, trp[i]), "Subtraction test is missing triples!");
+    for (size_t i = 3; i < 8; i++)
+        ASSERT (!LSUP_graph_contains (
+                    gr_dest, trp[i]), "Subtraction test has excess triples!");
+    LSUP_graph_free (gr_dest);
+
+    gr_dest = LSUP_graph_new (store, NULL, NULL);
+    EXPECT_PASS (LSUP_graph_bool_op (
+                LSUP_BOOL_SUBTRACTION, gr2, gr1, gr_dest));
+    for (size_t i = 0; i < 4; i++)
+        ASSERT (!LSUP_graph_contains (
+                    gr_dest, trp[i]), "Subtraction test is missing triples!");
+    for (size_t i = 4; i < 8; i++)
+        ASSERT (LSUP_graph_contains (
+                    gr_dest, trp[i]), "Subtraction test has excess triples!");
+    LSUP_graph_free (gr_dest);
+
+    // Test intersection.
+    gr_dest = LSUP_graph_new (store, NULL, NULL);
+    EXPECT_PASS (LSUP_graph_bool_op (
+                LSUP_BOOL_INTERSECTION, gr1, gr2, gr_dest));
+    for (size_t i = 0; i < 3; i++)
+        ASSERT (!LSUP_graph_contains (
+                    gr_dest, trp[i]), "Intersection is missing triples!");
+    ASSERT (LSUP_graph_contains (
+            gr_dest, trp[3]), "Intersection test failed!");
+    for (size_t i = 4; i < 8; i++)
+        ASSERT (!LSUP_graph_contains (
+                    gr_dest, trp[i]), "Intersection test has excess triples!");
+    LSUP_graph_free (gr_dest);
+
+    // Test XOR.
+    gr_dest = LSUP_graph_new (store, NULL, NULL);
+    EXPECT_PASS (LSUP_graph_bool_op (LSUP_BOOL_XOR, gr1, gr2, gr_dest));
+    for (size_t i = 0; i < 3; i++)
+        ASSERT (LSUP_graph_contains (
+                    gr_dest, trp[i]), "XOR test is missing triples!");
+    ASSERT (!LSUP_graph_contains (
+            gr_dest, trp[3]), "XOR test has excess triples!");
+    for (size_t i = 4; i < 8; i++)
+        ASSERT (LSUP_graph_contains (
+                    gr_dest, trp[i]), "XOR test is missing triples!");
+    LSUP_graph_free (gr_dest);
+
+    // Test union with result graph as one of the sources.
+    EXPECT_PASS (LSUP_graph_bool_op (LSUP_BOOL_UNION, gr1, gr2, gr1));
+
+    LSUP_graph_free (gr1);
+    LSUP_graph_free (gr2);
+    free_triples (trp);
+    if (type != LSUP_STORE_HTABLE) LSUP_store_free (store);
+
+    return 0;
+}
+
+
 static int
 _graph_lookup (LSUP_StoreType type)
 {
@@ -398,6 +501,16 @@ BACKEND_TBL
 }
 
 
+static int test_graph_bool_ops() {
+#define ENTRY(a, b) \
+    if (_graph_bool_ops (LSUP_STORE_##a) != 0) return -1;
+BACKEND_TBL
+#undef ENTRY
+
+    return 0;
+}
+
+
 static int test_graph_lookup() {
 #define ENTRY(a, b) \
     if (_graph_lookup (LSUP_STORE_##a) != 0) return -1;
@@ -480,6 +593,7 @@ int graph_tests()
     RUN (test_graph_new);
     RUN (test_graph_add);
     RUN (test_graph_get);
+    RUN (test_graph_bool_ops);
     RUN (test_graph_lookup);
     RUN (test_graph_remove);
     RUN (test_graph_copy);