From 3a4869df53b138127b75044d41ce18d94a016541 Mon Sep 17 00:00:00 2001 From: Callum Birks Date: Fri, 11 Oct 2024 12:18:24 +0100 Subject: [PATCH 1/9] Protect the expiration column check with file lock. --- LiteCore/Storage/SQLiteKeyStore.cc | 10 ++++++---- LiteCore/tests/c4BaseTest.cc | 29 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/LiteCore/Storage/SQLiteKeyStore.cc b/LiteCore/Storage/SQLiteKeyStore.cc index 69ebaf816..d42c5a617 100644 --- a/LiteCore/Storage/SQLiteKeyStore.cc +++ b/LiteCore/Storage/SQLiteKeyStore.cc @@ -522,10 +522,12 @@ namespace litecore { // 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")); + db().withFileLock([=]() { + 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; } diff --git a/LiteCore/tests/c4BaseTest.cc b/LiteCore/tests/c4BaseTest.cc index d79f8ba10..2d947e9f1 100644 --- a/LiteCore/tests/c4BaseTest.cc +++ b/LiteCore/tests/c4BaseTest.cc @@ -27,6 +27,8 @@ # include "Error.hh" # include #endif +#include +#include #include 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); From f3d089cb959d2e54362b46733fa681484105b52d Mon Sep 17 00:00:00 2001 From: Callum Birks Date: Fri, 11 Oct 2024 12:32:54 +0100 Subject: [PATCH 2/9] Formatting. --- LiteCore/tests/c4BaseTest.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/LiteCore/tests/c4BaseTest.cc b/LiteCore/tests/c4BaseTest.cc index 2d947e9f1..06b6bbba9 100644 --- a/LiteCore/tests/c4BaseTest.cc +++ b/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,7 +28,6 @@ # include "Error.hh" # include #endif -#include #include #include @@ -142,22 +142,22 @@ TEST_CASE("C4Error Reporting Macros", "[Errors][C]") { } TEST_CASE_METHOD(C4Test, "Create collection concurrently", "[Database][C]") { - const slice dbName = db->getName(); + 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 {}; + 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 }; + 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)); From 46732d5a40ebe51fbe7b565b1f61fb8eb56c5083 Mon Sep 17 00:00:00 2001 From: Callum Birks Date: Tue, 15 Oct 2024 15:57:17 +0100 Subject: [PATCH 3/9] Address PR comment. --- LiteCore/Storage/SQLiteKeyStore.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/LiteCore/Storage/SQLiteKeyStore.cc b/LiteCore/Storage/SQLiteKeyStore.cc index d42c5a617..57c800045 100644 --- a/LiteCore/Storage/SQLiteKeyStore.cc +++ b/LiteCore/Storage/SQLiteKeyStore.cc @@ -525,11 +525,11 @@ namespace litecore { db().withFileLock([=]() { 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")); + db().exec(subst("ALTER TABLE kv_@ ADD COLUMN expiration INTEGER; " + "CREATE INDEX \"kv_@_expiration\" ON kv_@ (expiration) WHERE expiration not null")); + _uncommittedExpirationColumn = true; }); - _hasExpirationColumn = true; - _uncommittedExpirationColumn = true; + _hasExpirationColumn = true; } bool SQLiteKeyStore::setExpiration(slice key, expiration_t expTime) { From 569c7abbb8936d9948a06c63494332e5aa1826f1 Mon Sep 17 00:00:00 2001 From: Callum Birks Date: Tue, 15 Oct 2024 16:10:20 +0100 Subject: [PATCH 4/9] Add early check for `addExpiration`. --- LiteCore/Storage/SQLiteKeyStore.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/LiteCore/Storage/SQLiteKeyStore.cc b/LiteCore/Storage/SQLiteKeyStore.cc index 57c800045..8154d0620 100644 --- a/LiteCore/Storage/SQLiteKeyStore.cc +++ b/LiteCore/Storage/SQLiteKeyStore.cc @@ -522,6 +522,7 @@ namespace litecore { // Adds the 'expiration' column to the table. void SQLiteKeyStore::addExpiration() { + if ( _hasExpirationColumn ) return; db().withFileLock([=]() { if ( mayHaveExpiration() ) return; db()._logVerbose("Adding the `expiration` column & index to kv_%s", name().c_str()); From 4e79b0b9db14a81fc2c30e2a6a24405bcb6474ac Mon Sep 17 00:00:00 2001 From: Callum Birks Date: Thu, 24 Oct 2024 13:54:52 +0100 Subject: [PATCH 5/9] Move `expiration` column to schema addition. --- LiteCore/Query/SQLiteQuery.cc | 5 ----- LiteCore/Storage/BothKeyStore.hh | 5 ----- LiteCore/Storage/KeyStore.hh | 4 +--- LiteCore/Storage/SQLiteDataFile.cc | 16 +++++++++++--- LiteCore/Storage/SQLiteDataFile.hh | 7 +++--- LiteCore/Storage/SQLiteKeyStore.cc | 35 ++++++++++-------------------- LiteCore/Storage/SQLiteKeyStore.hh | 4 ---- 7 files changed, 29 insertions(+), 47 deletions(-) diff --git a/LiteCore/Query/SQLiteQuery.cc b/LiteCore/Query/SQLiteQuery.cc index e5d97fdf2..f9038910d 100644 --- a/LiteCore/Query/SQLiteQuery.cc +++ b/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()); diff --git a/LiteCore/Storage/BothKeyStore.hh b/LiteCore/Storage/BothKeyStore.hh index 481d44789..20b3ee27b 100644 --- a/LiteCore/Storage/BothKeyStore.hh +++ b/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); } diff --git a/LiteCore/Storage/KeyStore.hh b/LiteCore/Storage/KeyStore.hh index 15a60b466..70493d24b 100644 --- a/LiteCore/Storage/KeyStore.hh +++ b/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; @@ -210,7 +208,7 @@ namespace litecore { // public for complicated reasons; clients should never call it virtual ~KeyStore() = default; - KeyStore(const KeyStore&) = delete; // not copyable + KeyStore(const KeyStore&) = delete; // not copyable KeyStore& operator=(const KeyStore&) = delete; protected: diff --git a/LiteCore/Storage/SQLiteDataFile.cc b/LiteCore/Storage/SQLiteDataFile.cc index be64a692e..4806cb0a6 100644 --- a/LiteCore/Storage/SQLiteDataFile.cc +++ b/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_ diff --git a/LiteCore/Storage/SQLiteDataFile.hh b/LiteCore/Storage/SQLiteDataFile.hh index 891d036b8..83ac92480 100644 --- a/LiteCore/Storage/SQLiteDataFile.hh +++ b/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 }; diff --git a/LiteCore/Storage/SQLiteKeyStore.cc b/LiteCore/Storage/SQLiteKeyStore.cc index 8154d0620..2cde48c4e 100644 --- a/LiteCore/Storage/SQLiteKeyStore.cc +++ b/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,22 +521,8 @@ namespace litecore { return _hasExpirationColumn; } - // Adds the 'expiration' column to the table. - void SQLiteKeyStore::addExpiration() { - if ( _hasExpirationColumn ) return; - db().withFileLock([=]() { - if ( mayHaveExpiration() ) return; - db()._logVerbose("Adding the `expiration` column & index to kv_%s", name().c_str()); - db().exec(subst("ALTER TABLE kv_@ ADD COLUMN expiration INTEGER; " - "CREATE INDEX \"kv_@_expiration\" ON kv_@ (expiration) WHERE expiration not null")); - _uncommittedExpirationColumn = true; - }); - _hasExpirationColumn = 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); diff --git a/LiteCore/Storage/SQLiteKeyStore.hh b/LiteCore/Storage/SQLiteKeyStore.hh index d1f3dd233..f9f398ede 100644 --- a/LiteCore/Storage/SQLiteKeyStore.hh +++ b/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 _lastSequence; mutable std::atomic _purgeCount{0}; bool _hasExpirationColumn{false}; - bool _uncommittedExpirationColumn{false}; bool _uncommitedTable{false}; SQLiteKeyStore* _sequencesOwner{nullptr}; }; From f1083da87b5a55cf9ac09af644152f2f94e9ac29 Mon Sep 17 00:00:00 2001 From: Callum Birks Date: Thu, 24 Oct 2024 14:06:00 +0100 Subject: [PATCH 6/9] Formatting. --- LiteCore/Storage/KeyStore.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/LiteCore/Storage/KeyStore.hh b/LiteCore/Storage/KeyStore.hh index 70493d24b..6e4b0bb6f 100644 --- a/LiteCore/Storage/KeyStore.hh +++ b/LiteCore/Storage/KeyStore.hh @@ -208,7 +208,7 @@ namespace litecore { // public for complicated reasons; clients should never call it virtual ~KeyStore() = default; - KeyStore(const KeyStore&) = delete; // not copyable + KeyStore(const KeyStore&) = delete; // not copyable KeyStore& operator=(const KeyStore&) = delete; protected: From 39444b2c0f4e306ddf259a736b7d67724fa08aa5 Mon Sep 17 00:00:00 2001 From: Callum Birks Date: Thu, 24 Oct 2024 14:23:33 +0100 Subject: [PATCH 7/9] Formatting. --- LiteCore/Storage/KeyStore.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/LiteCore/Storage/KeyStore.hh b/LiteCore/Storage/KeyStore.hh index 6e4b0bb6f..5d21f96f3 100644 --- a/LiteCore/Storage/KeyStore.hh +++ b/LiteCore/Storage/KeyStore.hh @@ -208,7 +208,7 @@ namespace litecore { // public for complicated reasons; clients should never call it virtual ~KeyStore() = default; - KeyStore(const KeyStore&) = delete; // not copyable + KeyStore(const KeyStore&) = delete; // not copyable KeyStore& operator=(const KeyStore&) = delete; protected: From df4af06cd8f3fc6d8a3ab1b00085e05227d8e741 Mon Sep 17 00:00:00 2001 From: Callum Birks Date: Thu, 24 Oct 2024 19:00:23 +0100 Subject: [PATCH 8/9] Check expiration column exists before adding. --- LiteCore/Storage/SQLiteDataFile.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/LiteCore/Storage/SQLiteDataFile.cc b/LiteCore/Storage/SQLiteDataFile.cc index 4806cb0a6..f081730fa 100644 --- a/LiteCore/Storage/SQLiteDataFile.cc +++ b/LiteCore/Storage/SQLiteDataFile.cc @@ -334,6 +334,12 @@ namespace litecore { // Add the 'expiration' column to every KeyStore: for ( string& name : allKeyStoreNames() ) { if ( name.find("::") == string::npos ) { + string sql; + // We need to check for existence of the expiration column first. + // Do not add it if it already exists in the table. + if ( getSchema("kv_" + name, "table", "kv_" + name, sql) + && sql.find("expiration") != string::npos ) + continue; // Only update data tables, not FTS index tables _exec(format( "ALTER TABLE \"kv_%s\" ADD COLUMN expiration INTEGER; " From dff750a2dd9538cdbb14afbd6fc581533a57c043 Mon Sep 17 00:00:00 2001 From: Callum Birks Date: Thu, 24 Oct 2024 23:08:03 +0100 Subject: [PATCH 9/9] Move `expiration` upgrade to schema v502. --- LiteCore/Storage/SQLiteDataFile.cc | 16 ++++++++-------- LiteCore/Storage/SQLiteDataFile.hh | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/LiteCore/Storage/SQLiteDataFile.cc b/LiteCore/Storage/SQLiteDataFile.cc index f081730fa..e8fd49a7b 100644 --- a/LiteCore/Storage/SQLiteDataFile.cc +++ b/LiteCore/Storage/SQLiteDataFile.cc @@ -330,6 +330,14 @@ namespace litecore { error::_throw(error::CantUpgradeDatabase); } + (void)upgradeSchema(SchemaVersion::WithIndexesLastSeq, "Adding indexes.lastSeq column", [&] { + string sql; + if ( getSchema("indexes", "table", "indexes", sql) ) { + // Check if the table needs to be updated to add the 'lastSeq' column: (v3.2) + if ( sql.find("lastSeq") == string::npos ) { _exec("ALTER TABLE indexes ADD COLUMN lastSeq TEXT"); } + } + }); + (void)upgradeSchema(SchemaVersion::WithExpirationColumn, "Adding `expiration` column", [&] { // Add the 'expiration' column to every KeyStore: for ( string& name : allKeyStoreNames() ) { @@ -348,14 +356,6 @@ namespace litecore { } } }); - - (void)upgradeSchema(SchemaVersion::WithIndexesLastSeq, "Adding indexes.lastSeq column", [&] { - string sql; - if ( getSchema("indexes", "table", "indexes", sql) ) { - // Check if the table needs to be updated to add the 'lastSeq' column: (v3.2) - if ( sql.find("lastSeq") == string::npos ) { _exec("ALTER TABLE indexes ADD COLUMN lastSeq TEXT"); } - } - }); }); // Configure number of extra threads to be used by SQLite: diff --git a/LiteCore/Storage/SQLiteDataFile.hh b/LiteCore/Storage/SQLiteDataFile.hh index 83ac92480..9f6964e85 100644 --- a/LiteCore/Storage/SQLiteDataFile.hh +++ b/LiteCore/Storage/SQLiteDataFile.hh @@ -174,12 +174,12 @@ namespace litecore { WithNewDocs = 400, // New document/revision storage (CBL 3.0) - 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) + WithExpirationColumn = 502, // Added 'expiration' column to KeyStore MaxReadable = 599, // Cannot open versions newer than this - Current = WithDeletedTable + Current = WithExpirationColumn }; void reopenSQLiteHandle();