CBL-6131: Race creating the expiration column in a collection table

LiteCore/Query/SQLiteQuery.cc

@@ -110,11 +110,6 @@ namespace litecore {
                     error::_throw(error::NoSuchIndex, "'match' test requires a full-text index");
             }
 
-            // If expiration is queried, ensure the table(s) have the expiration column:
-            if ( qp.usesExpiration() ) {
-                for ( auto ks : _keyStores ) ks->addExpiration();
-            }
-
             LogTo(SQL, "Compiled {Query#%u}: %s", getObjectRef(), sql.c_str());
             _statement = dataFile.compile(sql.c_str());
 

LiteCore/Storage/BothKeyStore.hh

@@ -67,11 +67,6 @@ namespace litecore {
 
         bool mayHaveExpiration() override { return _liveStore->mayHaveExpiration() || _deadStore->mayHaveExpiration(); }
 
-        void addExpiration() override {
-            _liveStore->addExpiration();
-            _deadStore->addExpiration();
-        }
-
         bool setExpiration(slice key, expiration_t exp) override {
             return _liveStore->setExpiration(key, exp) || _deadStore->setExpiration(key, exp);
         }

LiteCore/Storage/KeyStore.hh

@@ -164,8 +164,6 @@ namespace litecore {
         /** Does this KeyStore potentially have records that expire? (May return false positives.) */
         virtual bool mayHaveExpiration() = 0;
 
-        virtual void addExpiration() = 0;
-
         /** Sets a record's expiration time. Zero means 'never'.
             @return  true if the time was set, false if no record with that key exists. */
         virtual bool setExpiration(slice key, expiration_t) = 0;

LiteCore/Storage/SQLiteDataFile.cc

@@ -330,6 +330,19 @@ namespace litecore {
                 error::_throw(error::CantUpgradeDatabase);
             }
 
+            (void)upgradeSchema(SchemaVersion::WithExpirationColumn, "Adding `expiration` column", [&] {
+                // Add the 'expiration' column to every KeyStore:
+                for ( string& name : allKeyStoreNames() ) {
+                    if ( name.find("::") == string::npos ) {
+                        // Only update data tables, not FTS index tables
+                        _exec(format(
+                                "ALTER TABLE \"kv_%s\" ADD COLUMN expiration INTEGER; "
+                                "CREATE INDEX \"kv_%s_expiration\" ON \"kv_%s\" (expiration) WHERE expiration not null",
+                                name.c_str(), name.c_str(), name.c_str()));
+                    }
+                }
+            });
+
             (void)upgradeSchema(SchemaVersion::WithIndexesLastSeq, "Adding indexes.lastSeq column", [&] {
                 string sql;
                 if ( getSchema("indexes", "table", "indexes", sql) ) {
@@ -602,9 +615,6 @@ namespace litecore {
             // Wrap the store in a BothKeyStore that manages it and the deleted store:
             auto deletedStore = new SQLiteKeyStore(*this, kDeletedKeyStorePrefix + name, options);
 
-            keyStore->addExpiration();
-            deletedStore->addExpiration();
-
             // Create a SQLite view of a union of both stores, for use in queries:
 #define COLUMNS "key,sequence,flags,version,body,extra,expiration"
             // Invarient: keyStore->tablaName()     == kv_<tableName>

LiteCore/Storage/SQLiteDataFile.hh

@@ -174,9 +174,10 @@ namespace litecore {
 
             WithNewDocs = 400,  // New document/revision storage (CBL 3.0)
 
-            WithDeletedTable   = 500,  // Added 'deleted' KeyStore for deleted docs (CBL 3.0?)
-            WithIndexesLastSeq = 501,  // Added 'lastSeq' column to 'indexes' table (CBL 3.2)
-            MaxReadable        = 599,  // Cannot open versions newer than this
+            WithExpirationColumn = 490,  // Added 'expiration' column to KeyStore
+            WithDeletedTable     = 500,  // Added 'deleted' KeyStore for deleted docs (CBL 3.0?)
+            WithIndexesLastSeq   = 501,  // Added 'lastSeq' column to 'indexes' table (CBL 3.2)
+            MaxReadable          = 599,  // Cannot open versions newer than this
 
             Current = WithDeletedTable
         };

LiteCore/Storage/SQLiteKeyStore.cc

@@ -70,13 +70,16 @@ namespace litecore {
         // more efficient in SQLite to keep large columns at the end of a row.
         // Create the sequence and flags columns regardless of options, otherwise it's too
         // complicated to customize all the SQL queries to conditionally use them...
-        db().execWithLock(subst("CREATE TABLE IF NOT EXISTS kv_@ ("
-                                "  key TEXT PRIMARY KEY,"
-                                "  sequence INTEGER,"
-                                "  flags INTEGER DEFAULT 0,"
-                                "  version BLOB,"
-                                "  body BLOB,"
-                                "  extra BLOB)"));
+        db().execWithLock(
+                subst("CREATE TABLE IF NOT EXISTS kv_@ ("
+                      "  key TEXT PRIMARY KEY,"
+                      "  sequence INTEGER,"
+                      "  flags INTEGER DEFAULT 0,"
+                      "  version BLOB,"
+                      "  body BLOB,"
+                      "  expiration INTEGER,"
+                      "  extra BLOB);"
+                      "CREATE INDEX IF NOT EXISTS \"kv_@_expiration\" ON kv_@ (expiration) WHERE expiration not null"));
         _uncommitedTable = db().inTransaction();
     }
 
@@ -179,12 +182,10 @@ namespace litecore {
         _purgeCountValid = false;
 
         if ( !commit ) {
-            if ( _uncommittedExpirationColumn ) _hasExpirationColumn = false;
             if ( _uncommitedTable ) { close(); }
         }
 
-        _uncommittedExpirationColumn = false;
-        _uncommitedTable             = false;
+        _uncommitedTable = false;
     }
 
     /*static*/ slice SQLiteKeyStore::columnAsSlice(const SQLite::Column& col) {
@@ -520,19 +521,8 @@ namespace litecore {
         return _hasExpirationColumn;
     }
 
-    // Adds the 'expiration' column to the table.
-    void SQLiteKeyStore::addExpiration() {
-        if ( mayHaveExpiration() ) return;
-        db()._logVerbose("Adding the `expiration` column & index to kv_%s", name().c_str());
-        db().execWithLock(subst("ALTER TABLE kv_@ ADD COLUMN expiration INTEGER; "
-                                "CREATE INDEX \"kv_@_expiration\" ON kv_@ (expiration) WHERE expiration not null"));
-        _hasExpirationColumn         = true;
-        _uncommittedExpirationColumn = true;
-    }
-
     bool SQLiteKeyStore::setExpiration(slice key, expiration_t expTime) {
         Assert(expTime >= expiration_t(0), "Invalid (negative) expiration time");
-        addExpiration();
         auto&          stmt = compileCached("UPDATE kv_@ SET expiration=? WHERE key=?");
         UsingStatement u(stmt);
         if ( expTime > expiration_t::None ) stmt.bind(1, (long long)expTime);

LiteCore/Storage/SQLiteKeyStore.hh

@@ -90,9 +90,6 @@ namespace litecore {
         void createConflictsIndex();
         void createBlobsIndex();
 
-        /// Adds the `expiration` column to the table. Called only by SQLiteQuery.
-        void addExpiration() override;
-
         void shareSequencesWith(KeyStore&) override;
 
       protected:
@@ -161,7 +158,6 @@ namespace litecore {
         mutable std::optional<sequence_t> _lastSequence;
         mutable std::atomic<uint64_t>     _purgeCount{0};
         bool                              _hasExpirationColumn{false};
-        bool                              _uncommittedExpirationColumn{false};
         bool                              _uncommitedTable{false};
         SQLiteKeyStore*                   _sequencesOwner{nullptr};
     };

LiteCore/tests/c4BaseTest.cc

@@ -12,6 +12,7 @@
 
 #include "c4Test.hh"
 #include "c4Internal.hh"
+#include "c4Collection.h"
 #include "c4ExceptionUtils.hh"
 #include "fleece/InstanceCounted.hh"
 #include "catch.hpp"
@@ -27,6 +28,7 @@
 #    include "Error.hh"
 #    include <winerror.h>
 #endif
+#include <future>
 #include <sstream>
 
 using namespace fleece;
@@ -139,6 +141,33 @@ TEST_CASE("C4Error Reporting Macros", "[Errors][C]") {
 #endif
 }
 
+TEST_CASE_METHOD(C4Test, "Create collection concurrently", "[Database][C]") {
+    const slice             dbName = db->getName();
+    const C4DatabaseConfig2 config = db->getConfiguration();
+
+    c4::ref db2 = c4db_openNamed(dbName, &config, ERROR_INFO());
+    REQUIRE(db2);
+
+    char buf[6]{};
+    for ( int i = 0; i < 5; i++ ) {
+        C4Error err{};
+        C4Error err2{};
+
+        snprintf(buf, 6, "coll%i", i);
+
+        {
+            slice                  collName{buf};
+            const C4CollectionSpec spec{collName, "scope"_sl};
+
+            auto a1 = std::async(std::launch::async, c4db_createCollection, db, spec, ERROR_INFO(&err));
+            auto a2 = std::async(std::launch::async, c4db_createCollection, db2.get(), spec, ERROR_INFO(&err2));
+        }
+
+        CHECK(err.code == 0);
+        CHECK(err2.code == 0);
+    }
+}
+
 TEST_CASE_METHOD(C4Test, "Database Flag FullSync", "[Database][C]") {
     // Ensure that, by default, diskSyncFull is false.
     CHECK(!litecore::asInternal(db)->dataFile()->options().diskSyncFull);