* Enclose most operations that update the database in transactions.

* Open all database tables (Db objects) at initialisation time, not
  every time they are used.  This is necessary because tables have to
  outlive all transactions that refer to them.

6 files changed, 145 insertions, 96 deletions

diff --git a/src/db.cc b/src/db.cc
index 77ad93a86..61ecb203a 100644
--- a/src/db.cc
+++ b/src/db.cc
@@ -5,15 +5,6 @@
 
 
 /* Wrapper class to ensure proper destruction. */
-class DestroyDb
-{
-    Db * db;
-public:
-    DestroyDb(Db * _db) : db(_db) { }
-    ~DestroyDb() { db->close(0); delete db; }
-};
-
-
 class DestroyDbc 
 {
     Dbc * dbc;
@@ -38,55 +29,60 @@ Transaction::Transaction()
 Transaction::Transaction(Database & db)
 {
     db.requireEnv();
-    db.env->txn_begin(0, &txn, 0);
+    try {
+        db.env->txn_begin(0, &txn, 0);
+    } catch (DbException e) { rethrow(e); }
 }
 
 
 Transaction::~Transaction()
 {
-    if (txn) {
-        txn->abort();
-        txn = 0;
-    }
+    if (txn) abort();
 }
 
 
 void Transaction::commit()
 {
     if (!txn) throw Error("commit called on null transaction");
-    txn->commit(0);
+    debug(format("committing transaction %1%") % (void *) txn);
+    DbTxn * txn2 = txn;
     txn = 0;
+    try {
+        txn2->commit(0);
+    } catch (DbException e) { rethrow(e); }
 }
 
 
-void Database::requireEnv()
+void Transaction::abort()
 {
-    if (!env) throw Error("database environment not open");
+    if (!txn) throw Error("abort called on null transaction");
+    debug(format("aborting transaction %1%") % (void *) txn);
+    DbTxn * txn2 = txn;
+    txn = 0;
+    try {
+        txn2->abort();
+    } catch (DbException e) { rethrow(e); }
 }
 
 
-Db * Database::openDB(const Transaction & txn,
-    const string & table, bool create)
+void Database::requireEnv()
 {
-    requireEnv();
-
-    Db * db = new Db(env, 0);
+    if (!env) throw Error("database environment not open");
+}
 
-    try {
-        // !!! fixme when switching to BDB 4.1: use txn.
-        db->open(table.c_str(), 0, 
-            DB_HASH, create ? DB_CREATE : 0, 0666);
-    } catch (...) {
-        delete db;
-        throw;
-    }
 
-    return db;
+Db * Database::getDb(TableId table)
+{
+    map<TableId, Db *>::iterator i = tables.find(table);
+    if (i == tables.end())
+        throw Error("unknown table id");
+    return i->second;
 }
 
 
 Database::Database()
     : env(0)
+    , nextId(1)
 {
 }
 
@@ -95,8 +91,23 @@ Database::~Database()
 {
     if (env) {
         debug(format("closing database environment"));
-        env->txn_checkpoint(0, 0, 0);
-        env->close(0);
+
+        try {
+
+            for (map<TableId, Db *>::iterator i = tables.begin();
+                 i != tables.end(); i++)
+            {
+                debug(format("closing table %1%") % i->first);
+                Db * db = i->second;
+                db->close(0);
+                delete db;
+            }
+
+            env->txn_checkpoint(0, 0, 0);
+            env->close(0);
+
+        } catch (DbException e) { rethrow(e); }
+
         delete env;
     }
 }
@@ -112,8 +123,9 @@ void Database::open(const string & path)
 
         env->set_lg_bsize(32 * 1024); /* default */
         env->set_lg_max(256 * 1024); /* must be > 4 * lg_bsize */
+        env->set_lk_detect(DB_LOCK_DEFAULT);
         
-        env->open(path.c_str(), 
+        env->open(path.c_str(),
             DB_INIT_LOCK | DB_INIT_LOG | DB_INIT_MPOOL | DB_INIT_TXN |
             DB_CREATE,
             0666);
@@ -122,22 +134,36 @@ void Database::open(const string & path)
 }
 
 
-void Database::createTable(const string & table)
+TableId Database::openTable(const string & tableName)
 {
+    requireEnv();
+    TableId table = nextId++;
+
     try {
-        Db * db = openDB(noTxn, table, true);
-        DestroyDb destroyDb(db);
+
+        Db * db = new Db(env, 0);
+
+        try {
+            // !!! fixme when switching to BDB 4.1: use txn.
+            db->open(tableName.c_str(), 0, DB_HASH, DB_CREATE, 0666);
+        } catch (...) {
+            delete db;
+            throw;
+        }
+
+        tables[table] = db;
+
     } catch (DbException e) { rethrow(e); }
+
+    return table;
 }
 
 
-bool Database::queryString(const Transaction & txn, const string & table, 
+bool Database::queryString(const Transaction & txn, TableId table, 
     const string & key, string & data)
 {
     try {
-
-        Db * db = openDB(txn, table, false);
-        DestroyDb destroyDb(db);
+        Db * db = getDb(table);
 
         Dbt kt((void *) key.c_str(), key.length());
         Dbt dt;
@@ -156,7 +182,7 @@ bool Database::queryString(const Transaction & txn, const string & table,
 }
 
 
-bool Database::queryStrings(const Transaction & txn, const string & table, 
+bool Database::queryStrings(const Transaction & txn, TableId table, 
     const string & key, Strings & data)
 {
     string d;
@@ -190,13 +216,11 @@ bool Database::queryStrings(const Transaction & txn, const string & table,
 }
 
 
-void Database::setString(const Transaction & txn, const string & table,
+void Database::setString(const Transaction & txn, TableId table,
     const string & key, const string & data)
 {
     try {
-        Db * db = openDB(txn, table, false);
-        DestroyDb destroyDb(db);
-
+        Db * db = getDb(table);
         Dbt kt((void *) key.c_str(), key.length());
         Dbt dt((void *) data.c_str(), data.length());
         db->put(txn.txn, &kt, &dt, 0);
@@ -204,7 +228,7 @@ void Database::setString(const Transaction & txn, const string & table,
 }
 
 
-void Database::setStrings(const Transaction & txn, const string & table,
+void Database::setStrings(const Transaction & txn, TableId table,
     const string & key, const Strings & data)
 {
     string d;
@@ -227,28 +251,25 @@ void Database::setStrings(const Transaction & txn, const string & table,
 }
 
 
-void Database::delPair(const Transaction & txn, const string & table,
+void Database::delPair(const Transaction & txn, TableId table,
     const string & key)
 {
     try {
-        Db * db = openDB(txn, table, false);
-        DestroyDb destroyDb(db);
+        Db * db = getDb(table);
         Dbt kt((void *) key.c_str(), key.length());
         db->del(txn.txn, &kt, 0);
     } catch (DbException e) { rethrow(e); }
 }
 
 
-void Database::enumTable(const Transaction & txn, const string & table,
+void Database::enumTable(const Transaction & txn, TableId table,
     Strings & keys)
 {
     try {
-
-        Db * db = openDB(txn, table, false);
-        DestroyDb destroyDb(db);
+        Db * db = getDb(table);
 
         Dbc * dbc;
-        db->cursor(0, &dbc, 0);
+        db->cursor(txn.txn, &dbc, 0);
         DestroyDbc destroyDbc(dbc);
 
         Dbt kt, dt;
diff --git a/src/db.hh b/src/db.hh
index 57b6e4d8e..4bac943e5 100644
--- a/src/db.hh
+++ b/src/db.hh
@@ -3,6 +3,7 @@
 
 #include <string>
 #include <list>
+#include <map>
 
 #include <db_cxx.h>
 
@@ -26,6 +27,7 @@ public:
     Transaction(Database & _db);
     ~Transaction();
 
+    void abort();
     void commit();
 };
 
@@ -33,6 +35,9 @@ public:
 #define noTxn Transaction()
 
 
+typedef unsigned int TableId; /* table handles */
+
+
 class Database
 {
     friend class Transaction;
@@ -40,10 +45,12 @@ class Database
 private:
     DbEnv * env;
 
+    TableId nextId;
+    map<TableId, Db *> tables;
+
     void requireEnv();
 
-    Db * openDB(const Transaction & txn,
-        const string & table, bool create);
+    Db * getDb(TableId table);
 
 public:
     Database();
@@ -51,24 +58,24 @@ public:
     
     void open(const string & path);
 
-    void createTable(const string & table);
+    TableId openTable(const string & table);
 
-    bool queryString(const Transaction & txn, const string & table, 
+    bool queryString(const Transaction & txn, TableId table, 
         const string & key, string & data);
 
-    bool queryStrings(const Transaction & txn, const string & table, 
+    bool queryStrings(const Transaction & txn, TableId table, 
         const string & key, Strings & data);
 
-    void setString(const Transaction & txn, const string & table,
+    void setString(const Transaction & txn, TableId table,
         const string & key, const string & data);
 
-    void setStrings(const Transaction & txn, const string & table,
+    void setStrings(const Transaction & txn, TableId table,
         const string & key, const Strings & data);
 
-    void delPair(const Transaction & txn, const string & table,
+    void delPair(const Transaction & txn, TableId table,
         const string & key);
 
-    void enumTable(const Transaction & txn, const string & table,
+    void enumTable(const Transaction & txn, TableId table,
         Strings & keys);
 };
 
diff --git a/src/globals.cc b/src/globals.cc
index 8c3ec3828..1ec0c4f9b 100644
--- a/src/globals.cc
+++ b/src/globals.cc
@@ -5,10 +5,10 @@
 Database nixDB;
 
 
-string dbPath2Id = "path2id";
-string dbId2Paths = "id2paths";
-string dbSuccessors = "successors";
-string dbSubstitutes = "substitutes";
+TableId dbPath2Id;
+TableId dbId2Paths;
+TableId dbSuccessors;
+TableId dbSubstitutes;
 
 
 string nixStore = "/UNINIT";
@@ -20,13 +20,13 @@ string nixDBPath = "/UNINIT";
 void openDB()
 {
     nixDB.open(nixDBPath);
+    dbPath2Id = nixDB.openTable("path2id");
+    dbId2Paths = nixDB.openTable("id2paths");
+    dbSuccessors = nixDB.openTable("successors");
+    dbSubstitutes = nixDB.openTable("substitutes");
 }
 
 
 void initDB()
 {
-    nixDB.createTable(dbPath2Id);
-    nixDB.createTable(dbId2Paths);
-    nixDB.createTable(dbSuccessors);
-    nixDB.createTable(dbSubstitutes);
 }
diff --git a/src/globals.hh b/src/globals.hh
index 9df827622..2c4d33920 100644
--- a/src/globals.hh
+++ b/src/globals.hh
@@ -17,13 +17,13 @@ extern Database nixDB;
 
    Each pair (p, id) records that path $p$ contains an expansion of
    $id$. */
-extern string dbPath2Id;
+extern TableId dbPath2Id;
 
 
 /* dbId2Paths :: FSId -> [Path]
 
    A mapping from ids to lists of paths. */
-extern string dbId2Paths;
+extern TableId dbId2Paths;
 
 
 /* dbSuccessors :: FSId -> FSId
@@ -35,7 +35,7 @@ extern string dbId2Paths;
    Note that a term $y$ is successor of $x$ iff there exists a
    sequence of rewrite steps that rewrites $x$ into $y$.
 */
-extern string dbSuccessors;
+extern TableId dbSuccessors;
 
 
 /* dbSubstitutes :: FSId -> [FSId]
@@ -51,7 +51,7 @@ extern string dbSuccessors;
    this case might be an fstate expression that fetches the Nix
    archive. 
 */
-extern string dbSubstitutes;
+extern TableId dbSubstitutes;
 
 
 /* Path names. */
diff --git a/src/normalise.cc b/src/normalise.cc
index 5a8cb9a0d..e8fc6fc55 100644
--- a/src/normalise.cc
+++ b/src/normalise.cc
@@ -9,7 +9,9 @@
 
 void registerSuccessor(const FSId & id1, const FSId & id2)
 {
-    nixDB.setString(noTxn, dbSuccessors, id1, id2);
+    Transaction txn(nixDB);
+    nixDB.setString(txn, dbSuccessors, id1, id2);
+    txn.commit();
 }
 
 
diff --git a/src/store.cc b/src/store.cc
index 3dc625a7b..6d7861d0b 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -32,6 +32,8 @@ struct CopySource : RestoreSource
 
 void copyPath(string src, string dst)
 {
+    debug(format("copying `%1%' to `%2%'") % src % dst);
+
     /* Unfortunately C++ doesn't support coprocedures, so we have no
        nice way to chain CopySink and CopySource together.  Instead we
        fork off a child to run the sink.  (Fork-less platforms should
@@ -96,54 +98,69 @@ void registerSubstitute(const FSId & srcId, const FSId & subId)
     /* For now, accept only one substitute per id. */
     Strings subs;
     subs.push_back(subId);
-    nixDB.setStrings(noTxn, dbSubstitutes, srcId, subs);
+    
+    Transaction txn(nixDB);
+    nixDB.setStrings(txn, dbSubstitutes, srcId, subs);
+    txn.commit();
 }
 
 
 void registerPath(const string & _path, const FSId & id)
 {
     string path(canonPath(_path));
+    Transaction txn(nixDB);
 
-    nixDB.setString(noTxn, dbPath2Id, path, id);
+    debug(format("registering path `%1%' with id %2%")
+        % path % (string) id);
 
-    Strings paths;
-    nixDB.queryStrings(noTxn, dbId2Paths, id, paths); /* non-existence = ok */
+    string oldId;
+    if (nixDB.queryString(txn, dbPath2Id, path, oldId)) {
+        txn.abort();
+        if (id != parseHash(oldId))
+            throw Error(format("path `%1%' already contains id %2%")
+                % path % oldId);
+        return;
+    }
 
-    for (Strings::iterator it = paths.begin();
-         it != paths.end(); it++)
-        if (*it == path) return;
+    nixDB.setString(txn, dbPath2Id, path, id);
+
+    Strings paths;
+    nixDB.queryStrings(txn, dbId2Paths, id, paths); /* non-existence = ok */
     
     paths.push_back(path);
     
-    nixDB.setStrings(noTxn, dbId2Paths, id, paths);
+    nixDB.setStrings(txn, dbId2Paths, id, paths);
+
+    txn.commit();
 }
 
 
 void unregisterPath(const string & _path)
 {
     string path(canonPath(_path));
+    Transaction txn(nixDB);
+
+    debug(format("unregistering path `%1%'") % path);
 
     string _id;
-    if (!nixDB.queryString(noTxn, dbPath2Id, path, _id)) return;
+    if (!nixDB.queryString(txn, dbPath2Id, path, _id)) {
+        txn.abort();
+        return;
+    }
     FSId id(parseHash(_id));
 
-    nixDB.delPair(noTxn, dbPath2Id, path);
+    nixDB.delPair(txn, dbPath2Id, path);
 
-    /* begin transaction */
-    
     Strings paths, paths2;
-    nixDB.queryStrings(noTxn, dbId2Paths, id, paths); /* non-existence = ok */
+    nixDB.queryStrings(txn, dbId2Paths, id, paths); /* non-existence = ok */
 
-    bool changed = false;
     for (Strings::iterator it = paths.begin();
          it != paths.end(); it++)
-        if (*it != path) paths2.push_back(*it); else changed = true;
+        if (*it != path) paths2.push_back(*it);
 
-    if (changed)
-        nixDB.setStrings(noTxn, dbId2Paths, id, paths2);
-
-    /* end transaction */
+    nixDB.setStrings(txn, dbId2Paths, id, paths2);
 
+    txn.commit();
 }
 
 
@@ -230,6 +247,8 @@ string expandId(const FSId & id, const string & target,
 void addToStore(string srcPath, string & dstPath, FSId & id,
     bool deterministicName)
 {
+    debug(format("adding `%1%' to the store") % srcPath);
+
     srcPath = absPath(srcPath);
     id = hashPath(srcPath);