From 0d4c6e592e98e7f491f394bfa507dade405b5a62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Thu, 16 Mar 2023 20:03:20 +0100 Subject: [PATCH 01/11] SerializedDatabase.allowsUnsafeTransactions --- GRDB/Core/SerializedDatabase.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/GRDB/Core/SerializedDatabase.swift b/GRDB/Core/SerializedDatabase.swift index bcec3e9f75..9736ffde05 100644 --- a/GRDB/Core/SerializedDatabase.swift +++ b/GRDB/Core/SerializedDatabase.swift @@ -8,6 +8,9 @@ final class SerializedDatabase { /// The database configuration var configuration: Configuration { db.configuration } + /// If true, overrides `configuration.allowsUnsafeTransactions`. + var allowsUnsafeTransactions = false + /// The path to the database file let path: String @@ -242,7 +245,7 @@ final class SerializedDatabase { line: UInt = #line) { GRDBPrecondition( - configuration.allowsUnsafeTransactions || !db.isInsideTransaction, + allowsUnsafeTransactions || configuration.allowsUnsafeTransactions || !db.isInsideTransaction, message(), file: file, line: line) From e66dfacab82792f5d6df994f3d83224b268e50d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Thu, 16 Mar 2023 20:23:02 +0100 Subject: [PATCH 02/11] WALSnapshotTransaction --- GRDB.xcodeproj/project.pbxproj | 4 ++ GRDB/Core/DatabasePool.swift | 37 +++++++++++++ GRDB/Core/SerializedDatabase.swift | 2 + GRDB/Core/WALSnapshotTransaction.swift | 74 ++++++++++++++++++++++++++ GRDBCustom.xcodeproj/project.pbxproj | 4 ++ 5 files changed, 121 insertions(+) create mode 100644 GRDB/Core/WALSnapshotTransaction.swift diff --git a/GRDB.xcodeproj/project.pbxproj b/GRDB.xcodeproj/project.pbxproj index 1d711ca40a..43c7d6af69 100755 --- a/GRDB.xcodeproj/project.pbxproj +++ b/GRDB.xcodeproj/project.pbxproj @@ -281,6 +281,7 @@ 56D110D828AFC84000E64463 /* PersistableRecord+Insert.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D110D728AFC84000E64463 /* PersistableRecord+Insert.swift */; }; 56D110DD28AFC8B400E64463 /* PersistableRecord+Save.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D110DC28AFC8B400E64463 /* PersistableRecord+Save.swift */; }; 56D110FA28AFC97E00E64463 /* MutablePersistableRecord+DAO.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D110F928AFC97E00E64463 /* MutablePersistableRecord+DAO.swift */; }; + 56D3332029C38D6700430680 /* WALSnapshotTransaction.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D3331F29C38D6700430680 /* WALSnapshotTransaction.swift */; }; 56D496541D812F5B008276D7 /* SQLExpressionLiteralTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56A4CDAF1D4234B200B1A9B9 /* SQLExpressionLiteralTests.swift */; }; 56D496551D812F83008276D7 /* FoundationDataTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5657AB2F1D108BA9006283EF /* FoundationDataTests.swift */; }; 56D496571D81303E008276D7 /* FoundationDateComponentsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5690C3251D23E6D800E59934 /* FoundationDateComponentsTests.swift */; }; @@ -753,6 +754,7 @@ 56D110D728AFC84000E64463 /* PersistableRecord+Insert.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "PersistableRecord+Insert.swift"; sourceTree = ""; }; 56D110DC28AFC8B400E64463 /* PersistableRecord+Save.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "PersistableRecord+Save.swift"; sourceTree = ""; }; 56D110F928AFC97E00E64463 /* MutablePersistableRecord+DAO.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "MutablePersistableRecord+DAO.swift"; sourceTree = ""; }; + 56D3331F29C38D6700430680 /* WALSnapshotTransaction.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WALSnapshotTransaction.swift; sourceTree = ""; }; 56D5075D1F6BAE8600AE1C5B /* PrimaryKeyInfoTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PrimaryKeyInfoTests.swift; sourceTree = ""; }; 56D51CFF1EA789FA0074638A /* FetchableRecord+TableRecord.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "FetchableRecord+TableRecord.swift"; sourceTree = ""; }; 56D91AA12205E03700770D8D /* SQLRelation.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SQLRelation.swift; sourceTree = ""; }; @@ -1466,6 +1468,7 @@ 566B9C1F25C6CC24004542CF /* RowDecodingError.swift */, 56BB6EA81D3009B100A1CA52 /* SchedulingWatchdog.swift */, 560A37A61C8FF6E500949E71 /* SerializedDatabase.swift */, + 56D3331F29C38D6700430680 /* WALSnapshotTransaction.swift */, 56E9FAD7221053DC00C703A8 /* SQL.swift */, 569D6DDD220EF9E100A058A9 /* SQLInterpolation.swift */, 56FBFED82210731A00945324 /* SQLRequest.swift */, @@ -2157,6 +2160,7 @@ 5653EC122098738B00F46237 /* SQLGenerationContext.swift in Sources */, 560D92471C672C4B00F4F92B /* MutablePersistableRecord.swift in Sources */, 5674A6E41F307F0E0095F066 /* DatabaseValueConvertible+Decodable.swift in Sources */, + 56D3332029C38D6700430680 /* WALSnapshotTransaction.swift in Sources */, 5653EB0C20944C7C00F46237 /* HasManyAssociation.swift in Sources */, 5657AAB91D107001006283EF /* NSData.swift in Sources */, 560D92421C672C3E00F4F92B /* StatementColumnConvertible.swift in Sources */, diff --git a/GRDB/Core/DatabasePool.swift b/GRDB/Core/DatabasePool.swift index d9962c159f..57db0d254f 100644 --- a/GRDB/Core/DatabasePool.swift +++ b/GRDB/Core/DatabasePool.swift @@ -640,6 +640,43 @@ extension DatabasePool: DatabaseReader { return readers.first { $0.onValidQueue } } + // MARK: - WAL Snapshot Transactions + + // swiftlint:disable:next line_length +#if SQLITE_ENABLE_SNAPSHOT || (!GRDBCUSTOMSQLITE && !GRDBCIPHER && (compiler(>=5.7.1) || !(os(macOS) || targetEnvironment(macCatalyst)))) + /// Returns a long-lived WAL snapshot transaction on one of the + /// reader connections. + func walSnapshotTransaction() throws -> WALSnapshotTransaction { + guard let readerPool else { + throw DatabaseError.connectionIsClosed() + } + + let (reader, releaseReader) = try readerPool.get() + return try WALSnapshotTransaction(reader: reader) { transactionCompleted in + releaseReader(transactionCompleted ? .reuse : .discard) + } + } + + /// Returns a long-lived WAL snapshot transaction on one of the + /// reader connections. + func asyncWALSnapshotTransaction(_ completion: @escaping (Result) -> Void) { + guard let readerPool else { + completion(.failure(DatabaseError.connectionIsClosed())) + return + } + + readerPool.asyncGet { result in + completion(result.flatMap { reader, releaseReader in + Result { + try WALSnapshotTransaction(reader: reader) { transactionCompleted in + releaseReader(transactionCompleted ? .reuse : .discard) + } + } + }) + } + } +#endif + // MARK: - Database Observation public func _add( diff --git a/GRDB/Core/SerializedDatabase.swift b/GRDB/Core/SerializedDatabase.swift index 9736ffde05..76ba8d8a0b 100644 --- a/GRDB/Core/SerializedDatabase.swift +++ b/GRDB/Core/SerializedDatabase.swift @@ -9,6 +9,8 @@ final class SerializedDatabase { var configuration: Configuration { db.configuration } /// If true, overrides `configuration.allowsUnsafeTransactions`. + /// + /// See `WALSnapshotTransaction`. var allowsUnsafeTransactions = false /// The path to the database file diff --git a/GRDB/Core/WALSnapshotTransaction.swift b/GRDB/Core/WALSnapshotTransaction.swift new file mode 100644 index 0000000000..a3eb5e141f --- /dev/null +++ b/GRDB/Core/WALSnapshotTransaction.swift @@ -0,0 +1,74 @@ +// swiftlint:disable:next line_length +#if SQLITE_ENABLE_SNAPSHOT || (!GRDBCUSTOMSQLITE && !GRDBCIPHER && (compiler(>=5.7.1) || !(os(macOS) || targetEnvironment(macCatalyst)))) +/// A long-live read-only WAL transaction. +/// +/// `WALSnapshotTransaction` **takes ownership** of its reader +/// `SerializedDatabase` (TODO: make it a move-only type eventually). +class WALSnapshotTransaction { + private let reader: SerializedDatabase + private let transactionDidComplete: (Bool) -> Void + + /// The state of the database at the beginning of the transaction. + let walSnapshot: WALSnapshot + + /// Creates a long-live WAL transaction on a read-only connection. + /// + /// The `transactionDidComplete` closure is called when the + /// `WALSnapshotTransaction` is deallocated, or if the + /// initializer throws. Its argument tells if the long-lived + /// transaction could be properly terminated. + /// + /// - parameter reader: A read-only database connection. + /// - parameter transactionDidComplete: A closure to call when the + /// snapshot transaction ends. + init( + reader: SerializedDatabase, + transactionDidComplete: @escaping (Bool) -> Void) + throws + { + assert(reader.configuration.readonly) + + // Open a transaction and enter snapshot isolation + reader.allowsUnsafeTransactions = true + do { + try reader.sync { db in + try db.beginTransaction(.deferred) + try db.execute(sql: "SELECT rootpage FROM sqlite_master LIMIT 1") + } + } catch { + // self is not initialized, so deinit will not run. + Self.commitAndRelease(reader: reader, transactionDidComplete: transactionDidComplete) + throw error + } + + self.reader = reader + self.transactionDidComplete = transactionDidComplete + self.walSnapshot = try reader.sync { db in + return try WALSnapshot(db) + } + } + + deinit { + Self.commitAndRelease(reader: reader, transactionDidComplete: transactionDidComplete) + } + + /// Executes database operations in the snapshot transaction, and + /// returns their result after they have finished executing. + func read(_ value: (Database) throws -> T) rethrows -> T { + // TODO: we should check the validity of the snapshot, as DatabaseSnapshotPool does. + try reader.sync(value) + } + + private static func commitAndRelease( + reader: SerializedDatabase, + transactionDidComplete: (Bool) -> Void) + { + reader.allowsUnsafeTransactions = false + let success = reader.sync { db in + try? db.commit() + return db.isInsideTransaction + } + transactionDidComplete(success) + } +} +#endif diff --git a/GRDBCustom.xcodeproj/project.pbxproj b/GRDBCustom.xcodeproj/project.pbxproj index 3d8f50e2ff..84a9f56a36 100755 --- a/GRDBCustom.xcodeproj/project.pbxproj +++ b/GRDBCustom.xcodeproj/project.pbxproj @@ -276,6 +276,7 @@ 56D110F528AFC90800E64463 /* MutablePersistableRecord+Save.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D110E728AFC90800E64463 /* MutablePersistableRecord+Save.swift */; }; 56D110F728AFC90800E64463 /* MutablePersistableRecord+Update.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D110E828AFC90800E64463 /* MutablePersistableRecord+Update.swift */; }; 56D110FF28AFC9C600E64463 /* MutablePersistableRecord+DAO.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D110FE28AFC9C600E64463 /* MutablePersistableRecord+DAO.swift */; }; + 56D3332329C38D7B00430680 /* WALSnapshotTransaction.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D3332129C38D7A00430680 /* WALSnapshotTransaction.swift */; }; 56D507611F6BAE8600AE1C5B /* PrimaryKeyInfoTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D5075D1F6BAE8600AE1C5B /* PrimaryKeyInfoTests.swift */; }; 56D51D021EA789FA0074638A /* FetchableRecord+TableRecord.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56D51CFF1EA789FA0074638A /* FetchableRecord+TableRecord.swift */; }; 56DA7D03260FAA1B00A8D97B /* RecordMinimalNonOptionalPrimaryKeySingleTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56DA7D02260FAA1A00A8D97B /* RecordMinimalNonOptionalPrimaryKeySingleTests.swift */; }; @@ -763,6 +764,7 @@ 56D110E728AFC90800E64463 /* MutablePersistableRecord+Save.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "MutablePersistableRecord+Save.swift"; sourceTree = ""; }; 56D110E828AFC90800E64463 /* MutablePersistableRecord+Update.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "MutablePersistableRecord+Update.swift"; sourceTree = ""; }; 56D110FE28AFC9C600E64463 /* MutablePersistableRecord+DAO.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "MutablePersistableRecord+DAO.swift"; sourceTree = ""; }; + 56D3332129C38D7A00430680 /* WALSnapshotTransaction.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WALSnapshotTransaction.swift; sourceTree = ""; }; 56D5075D1F6BAE8600AE1C5B /* PrimaryKeyInfoTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PrimaryKeyInfoTests.swift; sourceTree = ""; }; 56D51CFF1EA789FA0074638A /* FetchableRecord+TableRecord.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "FetchableRecord+TableRecord.swift"; sourceTree = ""; }; 56DA7D02260FAA1A00A8D97B /* RecordMinimalNonOptionalPrimaryKeySingleTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RecordMinimalNonOptionalPrimaryKeySingleTests.swift; sourceTree = ""; }; @@ -1462,6 +1464,7 @@ 56231E6025CEBF06001DFD2F /* RowDecodingError.swift */, 56BB6EA81D3009B100A1CA52 /* SchedulingWatchdog.swift */, 560A37A61C8FF6E500949E71 /* SerializedDatabase.swift */, + 56D3332129C38D7A00430680 /* WALSnapshotTransaction.swift */, 56A6EB2226076F6A00C27594 /* SQL.swift */, 56E9FAC32210468500C703A8 /* SQLInterpolation.swift */, 56FBFED52210731000945324 /* SQLRequest.swift */, @@ -1981,6 +1984,7 @@ 56D110F328AFC90800E64463 /* MutablePersistableRecord.swift in Sources */, F3BA80831CFB2E67003DC1BA /* DatabaseValueConvertible+RawRepresentable.swift in Sources */, 5657AABB1D107001006283EF /* NSData.swift in Sources */, + 56D3332329C38D7B00430680 /* WALSnapshotTransaction.swift in Sources */, 5674A6E51F307F0E0095F066 /* DatabaseValueConvertible+Decodable.swift in Sources */, 5656A8652295BD56001FF3FF /* BelongsToAssociation.swift in Sources */, F3BA806A1CFB2E55003DC1BA /* DatabaseQueue.swift in Sources */, From d64274474b2d6e0c08ced8c3a8c2763c8e85c6f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Thu, 16 Mar 2023 20:25:05 +0100 Subject: [PATCH 03/11] ValueObservation: stop creating a new connection on start Since #1248, ValueObservation on DatabasePool creates a new database connection when it starts. #1256 has revealed that opening a connection can take too much time, which unfortunate consequences: the first value is notified too late, and we block the main thread when the observation is started in the `.immediate` scheduling :-/ In this commit, ValueObservation on DatabasePool just uses one of the available readers, thus avoiding creating a new connection. When WAL snapshots are available, a long-running read-only transaction is kept opened on this reader, until we can start observing the write transactions, compare database versions, and perform a fetch+notification if and only if the database was modified. When WAL snapshots are not available (Xcode 14.0 and macOS, or specific SQLCipher or SQLite builds), a double notification of the initial value is always performed, since we can't prove that the database was not changed between the initial fetch and the write access. --- .../Observers/ValueConcurrentObserver.swift | 249 ++++++++++++++---- 1 file changed, 205 insertions(+), 44 deletions(-) diff --git a/GRDB/ValueObservation/Observers/ValueConcurrentObserver.swift b/GRDB/ValueObservation/Observers/ValueConcurrentObserver.swift index 9508ee4a89..7211c20c0a 100644 --- a/GRDB/ValueObservation/Observers/ValueConcurrentObserver.swift +++ b/GRDB/ValueObservation/Observers/ValueConcurrentObserver.swift @@ -250,6 +250,16 @@ extension ValueConcurrentObserver { return AnyDatabaseCancellable(self) } + private func startObservation(_ writerDB: Database, observedRegion: DatabaseRegion) { + observationState.region = observedRegion + assert(observationState.isModified == false) + writerDB.add(transactionObserver: self, extent: .observerLifetime) + } +} + +// swiftlint:disable:next line_length +#if SQLITE_ENABLE_SNAPSHOT || (!GRDBCUSTOMSQLITE && !GRDBCIPHER && (compiler(>=5.7.1) || !(os(macOS) || targetEnvironment(macCatalyst)))) +extension ValueConcurrentObserver { /// Synchronously starts the observation, and returns the initial value. /// /// Unlike `asyncStart()`, this method does not notify the initial value or error. @@ -259,27 +269,22 @@ extension ValueConcurrentObserver { // without having to wait for an eventual long-running write // transaction to complete. // - // Fetch value & tracked region in a synchronous way. - // - // We perform the initial read from a DatabaseSnapshot, because - // it is a handy way to keep a read transaction open until we grab a - // write access, and compare the database versions. - let initialSnapshot = try databaseAccess.dbPool.makeSnapshot() - let (fetchedValue, initialRegion, initialWALSnapshot) = try initialSnapshot.read { - db -> (Reducer.Fetched, DatabaseRegion, WALSnapshot?) in - // swiftlint:disable:previous closure_parameter_position - + // We perform the initial read from a long-lived WAL snapshot + // transaction, because it is a handy way to keep a read transaction + // open until we grab a write access, and compare the database versions. + let walSnapshotTransaction = try databaseAccess.dbPool.walSnapshotTransaction() + let (fetchedValue, initialRegion) = try walSnapshotTransaction.read { db -> (Reducer.Fetched, DatabaseRegion) in switch trackingMode { case let .constantRegion(regions): let fetchedValue = try databaseAccess.fetch(db) let region = try DatabaseRegion.union(regions)(db) let initialRegion = try region.observableRegion(db) - return (fetchedValue, initialRegion, try? WALSnapshot(db)) + return (fetchedValue, initialRegion) case .constantRegionRecordedFromSelection, .nonConstantRegionRecordedFromSelection: let (fetchedValue, initialRegion) = try databaseAccess.fetchRecordingObservedRegion(db) - return (fetchedValue, initialRegion, try? WALSnapshot(db)) + return (fetchedValue, initialRegion) } } @@ -294,8 +299,7 @@ extension ValueConcurrentObserver { // Start observation asyncStartObservation( from: databaseAccess, - initialSnapshot: initialSnapshot, - initialWALSnapshot: initialWALSnapshot, + walSnapshotTransaction: walSnapshotTransaction, initialRegion: initialRegion) return initialValue @@ -310,20 +314,20 @@ extension ValueConcurrentObserver { // without having to wait for an eventual long-running write // transaction to complete. // - // We perform the initial read from a DatabaseSnapshot, because - // it is a handy way to keep a read transaction open until we grab a - // write access, and compare the database versions. - do { - let initialSnapshot = try databaseAccess.dbPool.makeSnapshot() - initialSnapshot.asyncRead { dbResult in - let isNotifying = self.lock.synchronized { self.notificationCallbacks != nil } - guard isNotifying else { return /* Cancelled */ } - - do { - // Fetch + // We perform the initial read from a long-lived WAL snapshot + // transaction, because it is a handy way to keep a read transaction + // open until we grab a write access, and compare the database versions. + databaseAccess.dbPool.asyncWALSnapshotTransaction { result in + let isNotifying = self.lock.synchronized { self.notificationCallbacks != nil } + guard isNotifying else { return /* Cancelled */ } + + do { + // Fetch + let walSnapshotTransaction = try result.get() + try walSnapshotTransaction.read { db in let fetchedValue: Reducer.Fetched let initialRegion: DatabaseRegion - let db = try dbResult.get() + switch self.trackingMode { case let .constantRegion(regions): fetchedValue = try databaseAccess.fetch(db) @@ -362,22 +366,18 @@ extension ValueConcurrentObserver { // Start observation self.asyncStartObservation( from: databaseAccess, - initialSnapshot: initialSnapshot, - initialWALSnapshot: try? WALSnapshot(db), + walSnapshotTransaction: walSnapshotTransaction, initialRegion: initialRegion) - } catch { - self.notifyError(error) } + } catch { + self.notifyError(error) } - } catch { - self.notifyError(error) } } private func asyncStartObservation( from databaseAccess: DatabaseAccess, - initialSnapshot: DatabaseSnapshot, - initialWALSnapshot: WALSnapshot?, + walSnapshotTransaction: WALSnapshotTransaction, initialRegion: DatabaseRegion) { databaseAccess.dbPool.asyncWriteWithoutTransaction { writerDB in @@ -389,17 +389,15 @@ extension ValueConcurrentObserver { // Transaction is needed for comparing version snapshots try writerDB.isolated(readOnly: true) { - // Keep initialSnapshot alive until we have compared + // Keep walSnapshotTransaction alive until we have compared // database versions. It prevents database checkpointing, // and keeps WAL snapshots (`sqlite3_snapshot`) valid // and comparable. - let isModified = withExtendedLifetime(initialSnapshot) { - guard let initialWALSnapshot, - let currentWALSnapshot = try? WALSnapshot(writerDB) - else { + let isModified = withExtendedLifetime(walSnapshotTransaction) { + guard let currentWALSnapshot = try? WALSnapshot(writerDB) else { return true } - let ordering = initialWALSnapshot.compare(currentWALSnapshot) + let ordering = walSnapshotTransaction.walSnapshot.compare(currentWALSnapshot) assert(ordering <= 0, "Unexpected snapshot ordering") return ordering < 0 } @@ -463,13 +461,176 @@ extension ValueConcurrentObserver { } } } +} +#else +extension ValueConcurrentObserver { + /// Synchronously starts the observation, and returns the initial value. + /// + /// Unlike `asyncStart()`, this method does not notify the initial value or error. + private func syncStart(from databaseAccess: DatabaseAccess) throws -> Reducer.Value { + // Start from a read access. The whole point of using a DatabasePool + // for observing the database is to be able to fetch the initial value + // without having to wait for an eventual long-running write + // transaction to complete. + let (fetchedValue, initialRegion) = try databaseAccess.dbPool.read { db -> (Reducer.Fetched, DatabaseRegion) in + switch trackingMode { + case let .constantRegion(regions): + let fetchedValue = try databaseAccess.fetch(db) + let region = try DatabaseRegion.union(regions)(db) + let initialRegion = try region.observableRegion(db) + return (fetchedValue, initialRegion) + + case .constantRegionRecordedFromSelection, + .nonConstantRegionRecordedFromSelection: + let (fetchedValue, initialRegion) = try databaseAccess.fetchRecordingObservedRegion(db) + return (fetchedValue, initialRegion) + } + } + + // Reduce + let initialValue = try reduceQueue.sync { + guard let initialValue = try reducer._value(fetchedValue) else { + fatalError("Broken contract: reducer has no initial value") + } + return initialValue + } + + // Start observation + asyncStartObservation( + from: databaseAccess, + initialRegion: initialRegion) + + return initialValue + } - private func startObservation(_ writerDB: Database, observedRegion: DatabaseRegion) { - observationState.region = observedRegion - assert(observationState.isModified == false) - writerDB.add(transactionObserver: self, extent: .observerLifetime) + /// Asynchronously starts the observation + /// + /// Unlike `syncStart()`, this method does notify the initial value or error. + private func asyncStart(from databaseAccess: DatabaseAccess) { + // Start from a read access. The whole point of using a DatabasePool + // for observing the database is to be able to fetch the initial value + // without having to wait for an eventual long-running write + // transaction to complete. + databaseAccess.dbPool.asyncRead { dbResult in + let isNotifying = self.lock.synchronized { self.notificationCallbacks != nil } + guard isNotifying else { return /* Cancelled */ } + + do { + // Fetch + let fetchedValue: Reducer.Fetched + let initialRegion: DatabaseRegion + let db = try dbResult.get() + switch self.trackingMode { + case let .constantRegion(regions): + fetchedValue = try databaseAccess.fetch(db) + let region = try DatabaseRegion.union(regions)(db) + initialRegion = try region.observableRegion(db) + + case .constantRegionRecordedFromSelection, + .nonConstantRegionRecordedFromSelection: + (fetchedValue, initialRegion) = try databaseAccess.fetchRecordingObservedRegion(db) + } + + // Reduce + // + // Reducing is performed asynchronously, so that we do not lock + // a database dispatch queue longer than necessary. + self.reduceQueue.async { + let isNotifying = self.lock.synchronized { self.notificationCallbacks != nil } + guard isNotifying else { return /* Cancelled */ } + + do { + guard let initialValue = try self.reducer._value(fetchedValue) else { + fatalError("Broken contract: reducer has no initial value") + } + + // Notify + self.scheduler.schedule { + let onChange = self.lock.synchronized { self.notificationCallbacks?.onChange } + guard let onChange else { return /* Cancelled */ } + onChange(initialValue) + } + } catch { + self.notifyError(error) + } + } + + // Start observation + self.asyncStartObservation( + from: databaseAccess, + initialRegion: initialRegion) + } catch { + self.notifyError(error) + } + } + } + + private func asyncStartObservation( + from databaseAccess: DatabaseAccess, + initialRegion: DatabaseRegion) + { + databaseAccess.dbPool.asyncWriteWithoutTransaction { writerDB in + let events = self.lock.synchronized { self.notificationCallbacks?.events } + guard let events else { return /* Cancelled */ } + events.databaseDidChange?() + + do { + try writerDB.isolated(readOnly: true) { + // Fetch + let fetchedValue: Reducer.Fetched + let observedRegion: DatabaseRegion + switch self.trackingMode { + case .constantRegion: + fetchedValue = try databaseAccess.fetch(writerDB) + observedRegion = initialRegion + events.willTrackRegion?(initialRegion) + self.startObservation(writerDB, observedRegion: initialRegion) + + case .constantRegionRecordedFromSelection, + .nonConstantRegionRecordedFromSelection: + (fetchedValue, observedRegion) = try databaseAccess.fetchRecordingObservedRegion(writerDB) + events.willTrackRegion?(observedRegion) + self.startObservation(writerDB, observedRegion: observedRegion) + } + + // Reduce + // + // Reducing is performed asynchronously, so that we do not lock + // the writer dispatch queue longer than necessary. + // + // Important: reduceQueue.async guarantees the same ordering + // between transactions and notifications! + self.reduceQueue.async { + let isNotifying = self.lock.synchronized { self.notificationCallbacks != nil } + guard isNotifying else { return /* Cancelled */ } + + do { + let value = try self.reducer._value(fetchedValue) + + // Notify + if let value { + self.scheduler.schedule { + let onChange = self.lock.synchronized { self.notificationCallbacks?.onChange } + guard let onChange else { return /* Cancelled */ } + onChange(value) + } + } + } catch { + let dbPool = self.lock.synchronized { self.databaseAccess?.dbPool } + dbPool?.asyncWriteWithoutTransaction { writerDB in + self.stopDatabaseObservation(writerDB) + } + self.notifyError(error) + } + } + } + } catch { + self.notifyError(error) + } + } } } +#endif // MARK: - Observing Database Transactions From 0a544dc0d2c04f31246290adbcaa63adc726a726 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Fri, 17 Mar 2023 08:29:06 +0100 Subject: [PATCH 04/11] Cleanup relationships between SerializedDatabase, WALSnapshotTransaction, and DatabasePool SerializedDatabase hides its allowsUnsafeTransactions property, and makes it thread-safe by protecting it inside its serialized dispatch queue. WALSnapshotTransaction always notifies when the reader connection is no longer used, and reuses the "isInsideTransaction" wording. --- GRDB/Core/DatabasePool.swift | 24 +++++---- GRDB/Core/SerializedDatabase.swift | 35 +++++++++---- GRDB/Core/WALSnapshotTransaction.swift | 57 ++++++++++++--------- Tests/GRDBTests/ValueObservationTests.swift | 4 +- 4 files changed, 73 insertions(+), 47 deletions(-) diff --git a/GRDB/Core/DatabasePool.swift b/GRDB/Core/DatabasePool.swift index 57db0d254f..5f49f41c36 100644 --- a/GRDB/Core/DatabasePool.swift +++ b/GRDB/Core/DatabasePool.swift @@ -644,21 +644,22 @@ extension DatabasePool: DatabaseReader { // swiftlint:disable:next line_length #if SQLITE_ENABLE_SNAPSHOT || (!GRDBCUSTOMSQLITE && !GRDBCIPHER && (compiler(>=5.7.1) || !(os(macOS) || targetEnvironment(macCatalyst)))) - /// Returns a long-lived WAL snapshot transaction on one of the - /// reader connections. + /// Returns a long-lived WAL snapshot transaction on a reader connection. func walSnapshotTransaction() throws -> WALSnapshotTransaction { guard let readerPool else { throw DatabaseError.connectionIsClosed() } let (reader, releaseReader) = try readerPool.get() - return try WALSnapshotTransaction(reader: reader) { transactionCompleted in - releaseReader(transactionCompleted ? .reuse : .discard) - } + return try WALSnapshotTransaction(onReader: reader, release: { isInsideTransaction in + // Discard the connection if the transaction could not be + // properly ended. If we'd reuse it, the next read would + // fail because we'd fail starting a read transaction. + releaseReader(isInsideTransaction ? .discard : .reuse) + }) } - /// Returns a long-lived WAL snapshot transaction on one of the - /// reader connections. + /// Returns a long-lived WAL snapshot transaction on a reader connection. func asyncWALSnapshotTransaction(_ completion: @escaping (Result) -> Void) { guard let readerPool else { completion(.failure(DatabaseError.connectionIsClosed())) @@ -668,9 +669,12 @@ extension DatabasePool: DatabaseReader { readerPool.asyncGet { result in completion(result.flatMap { reader, releaseReader in Result { - try WALSnapshotTransaction(reader: reader) { transactionCompleted in - releaseReader(transactionCompleted ? .reuse : .discard) - } + try WALSnapshotTransaction(onReader: reader, release: { isInsideTransaction in + // Discard the connection if the transaction could not be + // properly ended. If we'd reuse it, the next read would + // fail because we'd fail starting a read transaction. + releaseReader(isInsideTransaction ? .discard : .reuse) + }) } }) } diff --git a/GRDB/Core/SerializedDatabase.swift b/GRDB/Core/SerializedDatabase.swift index 76ba8d8a0b..482a34978c 100644 --- a/GRDB/Core/SerializedDatabase.swift +++ b/GRDB/Core/SerializedDatabase.swift @@ -8,17 +8,15 @@ final class SerializedDatabase { /// The database configuration var configuration: Configuration { db.configuration } - /// If true, overrides `configuration.allowsUnsafeTransactions`. - /// - /// See `WALSnapshotTransaction`. - var allowsUnsafeTransactions = false - /// The path to the database file let path: String /// The dispatch queue private let queue: DispatchQueue + /// If true, overrides `configuration.allowsUnsafeTransactions`. + private var allowsUnsafeTransactions = false + init( path: String, configuration: Configuration = Configuration(), @@ -81,10 +79,25 @@ final class SerializedDatabase { } } - /// Synchronously executes a block the serialized dispatch queue, and - /// returns its result. + /// Executes database operations, returns their result after they have + /// finished executing, and allows or forbids long-lived transactions. + /// + /// This method is not reentrant. + /// + /// - parameter allowingLongLivedTransaction: When true, the + /// ``Configuration/allowsUnsafeTransactions`` configuration flag is + /// ignored until this method is called again with false. + func sync(allowingLongLivedTransaction: Bool, _ body: (Database) throws -> T) rethrows -> T { + try sync { db in + self.allowsUnsafeTransactions = allowingLongLivedTransaction + return try body(db) + } + } + + /// Executes database operations, and returns their result after they + /// have finished executing. /// - /// This method is *not* reentrant. + /// This method is not reentrant. func sync(_ block: (Database) throws -> T) rethrows -> T { // Three different cases: // @@ -127,8 +140,8 @@ final class SerializedDatabase { } } - /// Synchronously executes a block the serialized dispatch queue, and - /// returns its result. + /// Executes database operations, and returns their result after they + /// have finished executing. /// /// This method is reentrant. func reentrantSync(_ block: (Database) throws -> T) rethrows -> T { @@ -194,7 +207,7 @@ final class SerializedDatabase { } } - /// Asynchronously executes a block in the serialized dispatch queue. + // Schedules database operations for execution, and returns immediately. func async(_ block: @escaping (Database) -> Void) { queue.async { block(self.db) diff --git a/GRDB/Core/WALSnapshotTransaction.swift b/GRDB/Core/WALSnapshotTransaction.swift index a3eb5e141f..fb0323cec5 100644 --- a/GRDB/Core/WALSnapshotTransaction.swift +++ b/GRDB/Core/WALSnapshotTransaction.swift @@ -6,50 +6,60 @@ /// `SerializedDatabase` (TODO: make it a move-only type eventually). class WALSnapshotTransaction { private let reader: SerializedDatabase - private let transactionDidComplete: (Bool) -> Void + private let release: (_ isInsideTransaction: Bool) -> Void /// The state of the database at the beginning of the transaction. let walSnapshot: WALSnapshot /// Creates a long-live WAL transaction on a read-only connection. /// - /// The `transactionDidComplete` closure is called when the - /// `WALSnapshotTransaction` is deallocated, or if the - /// initializer throws. Its argument tells if the long-lived - /// transaction could be properly terminated. + /// The `release` closure is always called. It is called when the + /// `WALSnapshotTransaction` is deallocated, or if the initializer + /// throws. + /// + /// In normal operations, the argument to `release` is always false, + /// meaning that the connection is no longer in a transaction. If true, + /// the connection has been left inside a transaction, due to + /// some error. + /// + /// Usage: + /// + /// ```swift + /// let transaction = WALSnapshotTransaction( + /// reader: reader, + /// release: { isInsideTransaction in + /// ... + /// }) + /// ``` /// /// - parameter reader: A read-only database connection. - /// - parameter transactionDidComplete: A closure to call when the - /// snapshot transaction ends. + /// - parameter release: A closure to call when the read-only connection + /// is no longer used. init( - reader: SerializedDatabase, - transactionDidComplete: @escaping (Bool) -> Void) + onReader reader: SerializedDatabase, + release: @escaping (_ isInsideTransaction: Bool) -> Void) throws { assert(reader.configuration.readonly) - // Open a transaction and enter snapshot isolation - reader.allowsUnsafeTransactions = true do { - try reader.sync { db in + // Open a long-lived transaction, and enter snapshot isolation + self.walSnapshot = try reader.sync(allowingLongLivedTransaction: true) { db in try db.beginTransaction(.deferred) try db.execute(sql: "SELECT rootpage FROM sqlite_master LIMIT 1") + return try WALSnapshot(db) } + self.reader = reader + self.release = release } catch { // self is not initialized, so deinit will not run. - Self.commitAndRelease(reader: reader, transactionDidComplete: transactionDidComplete) + Self.commitAndRelease(reader: reader, release: release) throw error } - - self.reader = reader - self.transactionDidComplete = transactionDidComplete - self.walSnapshot = try reader.sync { db in - return try WALSnapshot(db) - } } deinit { - Self.commitAndRelease(reader: reader, transactionDidComplete: transactionDidComplete) + Self.commitAndRelease(reader: reader, release: release) } /// Executes database operations in the snapshot transaction, and @@ -61,14 +71,13 @@ class WALSnapshotTransaction { private static func commitAndRelease( reader: SerializedDatabase, - transactionDidComplete: (Bool) -> Void) + release: (_ isInsideTransaction: Bool) -> Void) { - reader.allowsUnsafeTransactions = false - let success = reader.sync { db in + let isInsideTransaction = reader.sync(allowingLongLivedTransaction: false) { db in try? db.commit() return db.isInsideTransaction } - transactionDidComplete(success) + release(isInsideTransaction) } } #endif diff --git a/Tests/GRDBTests/ValueObservationTests.swift b/Tests/GRDBTests/ValueObservationTests.swift index 754b730464..798b389e47 100644 --- a/Tests/GRDBTests/ValueObservationTests.swift +++ b/Tests/GRDBTests/ValueObservationTests.swift @@ -412,8 +412,8 @@ class ValueObservationTests: GRDBTestCase { DispatchQueue.main.asyncAfter(deadline: .now() + 1) { try! dbPool.write { db in try db.execute(sql: """ - INSERT INTO t DEFAULT VALUES; - """) + INSERT INTO t DEFAULT VALUES; + """) } } } From 2f4d0c4588dc471a8a6e74e119cf2f0f42793fc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Fri, 17 Mar 2023 08:41:22 +0100 Subject: [PATCH 05/11] CHANGELOG --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4187848cc..ff9744e3a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -107,6 +107,10 @@ GRDB adheres to [Semantic Versioning](https://semver.org/), with one exception: --- +## Next Release + +- **New**: [#1350](https://github.com/groue/GRDB.swift/pull/1350) by [@groue](https://github.com/groue): ValueObservation on DatabasePool no longer opens a new database connection when it starts + ## 6.9.2 Released March 14, 2023 • [diff](https://github.com/groue/GRDB.swift/compare/v6.9.1...v6.9.2) From 063877d49cefa8ac5c26621efc633b2b3b64ae8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Rou=C3=A9?= Date: Fri, 17 Mar 2023 13:28:49 +0100 Subject: [PATCH 06/11] Configuration.persistentReaderConnections --- GRDB/Core/Configuration.swift | 25 ++- GRDB/Core/DatabasePool.swift | 63 ++++-- .../Extension/Configuration.md | 1 + .../DatabasePoolReleaseMemoryTests.swift | 200 +++++++++++------- 4 files changed, 190 insertions(+), 99 deletions(-) diff --git a/GRDB/Core/Configuration.swift b/GRDB/Core/Configuration.swift index db85192f21..f2868d108d 100644 --- a/GRDB/Core/Configuration.swift +++ b/GRDB/Core/Configuration.swift @@ -301,7 +301,7 @@ public struct Configuration { /// If nil, GRDB picks a default one. var readonlyBusyMode: Database.BusyMode? = nil - /// The maximum number of concurrent readers. + /// The maximum number of concurrent read-only connections. /// /// This configuration applies to ``DatabasePool`` only. The default value /// is 5. @@ -372,7 +372,28 @@ public struct Configuration { /// The default is true. public var automaticMemoryManagement = true #endif - + + /// A boolean value indicating whether read-only connections should be + /// kept open as long as they remain in a valid state. + /// + /// This configuration applies to ``DatabasePool`` only. The default value + /// is false. + /// + /// A `DatabasePool` automatically closes read-only connections on + /// various occasions, in order to spare memory + /// (see ``DatabasePool/releaseMemory()``), or when connections enter an + /// invalid state. + /// + /// When this flag is true, only invalid connections are automatically + /// closed. Valid connections, once opened, are kept alive until the + /// `DatabasePool` is deinitialized, or one of those methods is called: + /// ``DatabaseReader/close()``, + /// ``DatabasePool/invalidateReadOnlyConnections()``. + /// + /// Consider using this flag when profiling your application reveals + /// that a lot of time is spent opening new SQLite connections. + public var persistentReaderConnections = false + // MARK: - Factory Configuration /// Creates a factory configuration. diff --git a/GRDB/Core/DatabasePool.swift b/GRDB/Core/DatabasePool.swift index 5f49f41c36..b08a139061 100644 --- a/GRDB/Core/DatabasePool.swift +++ b/GRDB/Core/DatabasePool.swift @@ -168,12 +168,15 @@ extension DatabasePool { // MARK: - Memory management - /// Frees as much memory as possible, by disposing non-essential memory from - /// the writer connection, and closing all reader connections. + /// Frees as much memory as possible, by disposing non-essential memory. /// /// This method is synchronous, and blocks the current thread until all /// database accesses are completed. /// + /// This method closes all reader connections, unless the + /// ``Configuration/persistentReaderConnections`` configuration flag + /// is set. + /// /// - warning: This method can prevent concurrent reads from executing, /// until it returns. Prefer ``releaseMemoryEventually()`` if you intend /// to keep on using the database while releasing memory. @@ -181,34 +184,50 @@ extension DatabasePool { // Release writer memory writer.sync { $0.releaseMemory() } - // Release readers memory by closing all connections. - // - // We must use a barrier in order to guarantee that memory has been - // freed (reader connections closed) when the method exits, as - // documented. - // - // Without the barrier, connections would only close _eventually_ (after - // their eventual concurrent jobs have completed). - readerPool?.barrier { - readerPool?.removeAll() + if configuration.persistentReaderConnections { + // Keep existing readers + readerPool?.forEach { reader in + reader.sync { $0.releaseMemory() } + } + } else { + // Release readers memory by closing all connections. + // + // We must use a barrier in order to guarantee that memory has been + // freed (reader connections closed) when the method exits, as + // documented. + // + // Without the barrier, connections would only close _eventually_ (after + // their eventual concurrent jobs have completed). + readerPool?.barrier { + readerPool?.removeAll() + } } } - /// Eventually frees as much memory as possible, by disposing non-essential - /// memory from the writer connection, and closing all reader connections. + /// Eventually frees as much memory as possible, by disposing + /// non-essential memory. + /// + /// This method eventually closes all reader connections, unless the + /// ``Configuration/persistentReaderConnections`` configuration flag + /// is set. /// /// Unlike ``releaseMemory()``, this method does not prevent concurrent /// database accesses when it is executing. But it does not notify when /// non-essential memory has been freed. public func releaseMemoryEventually() { - // Release readers memory by eventually closing all reader connections - // (they will close after their current jobs have completed). - readerPool?.removeAll() + if configuration.persistentReaderConnections { + // Keep existing readers + readerPool?.forEach { reader in + reader.async { $0.releaseMemory() } + } + } else { + // Release readers memory by eventually closing all reader connections + // (they will close after their current jobs have completed). + readerPool?.removeAll() + } // Release writer memory eventually. - writer.async { db in - db.releaseMemory() - } + writer.async { $0.releaseMemory() } } #if os(iOS) @@ -610,6 +629,10 @@ extension DatabasePool: DatabaseReader { /// /// Eventual concurrent read-only accesses are not invalidated: they will /// proceed until completion. + /// + /// - This method closes all read-only connections, even if the + /// ``Configuration/persistentReaderConnections`` configuration flag + /// is set. public func invalidateReadOnlyConnections() { readerPool?.removeAll() } diff --git a/GRDB/Documentation.docc/Extension/Configuration.md b/GRDB/Documentation.docc/Extension/Configuration.md index 87464a9e7d..aaf64e58fd 100644 --- a/GRDB/Documentation.docc/Extension/Configuration.md +++ b/GRDB/Documentation.docc/Extension/Configuration.md @@ -92,6 +92,7 @@ do { - ``label`` - ``maximumReaderCount`` - ``observesSuspensionNotifications`` +- ``persistentReaderConnections`` - ``prepareDatabase(_:)`` - ``publicStatementArguments`` - ``transactionClock`` diff --git a/Tests/GRDBTests/DatabasePoolReleaseMemoryTests.swift b/Tests/GRDBTests/DatabasePoolReleaseMemoryTests.swift index 30df8d81b7..31e26faa8f 100644 --- a/Tests/GRDBTests/DatabasePoolReleaseMemoryTests.swift +++ b/Tests/GRDBTests/DatabasePoolReleaseMemoryTests.swift @@ -120,83 +120,129 @@ class DatabasePoolReleaseMemoryTests: GRDBTestCase { #endif - // TODO: fix flaky test -// func testDatabasePoolReleaseMemoryClosesReaderConnections() throws { -// let countQueue = DispatchQueue(label: "GRDB") -// var openConnectionCount = 0 -// var totalOpenConnectionCount = 0 -// -// dbConfiguration.SQLiteConnectionDidOpen = { -// countQueue.sync { -// totalOpenConnectionCount += 1 -// openConnectionCount += 1 -// } -// } -// -// dbConfiguration.SQLiteConnectionDidClose = { -// countQueue.sync { -// openConnectionCount -= 1 -// } -// } -// -// let dbPool = try makeDatabasePool() -// try dbPool.write { db in -// try db.execute(sql: "CREATE TABLE items (id INTEGER PRIMARY KEY)") -// for _ in 0..<2 { -// try db.execute(sql: "INSERT INTO items (id) VALUES (NULL)") -// } -// } -// -// // Block 1 Block 2 Block3 -// // SELECT * FROM items -// // step -// // > -// let s1 = DispatchSemaphore(value: 0) -// // SELECT * FROM items -// // step -// // > -// let s2 = DispatchSemaphore(value: 0) -// // step step -// // > -// let s3 = DispatchSemaphore(value: 0) -// // end end releaseMemory -// -// let block1 = { () in -// try! dbPool.read { db in -// let cursor = try Row.fetchCursor(db, sql: "SELECT * FROM items") -// XCTAssertTrue(try cursor.next() != nil) -// s1.signal() -// _ = s2.wait(timeout: .distantFuture) -// XCTAssertTrue(try cursor.next() != nil) -// s3.signal() -// XCTAssertTrue(try cursor.next() == nil) -// } -// } -// let block2 = { () in -// _ = s1.wait(timeout: .distantFuture) -// try! dbPool.read { db in -// let cursor = try Row.fetchCursor(db, sql: "SELECT * FROM items") -// XCTAssertTrue(try cursor.next() != nil) -// s2.signal() -// XCTAssertTrue(try cursor.next() != nil) -// XCTAssertTrue(try cursor.next() == nil) -// } -// } -// let block3 = { () in -// _ = s3.wait(timeout: .distantFuture) -// dbPool.releaseMemory() -// } -// let blocks = [block1, block2, block3] -// DispatchQueue.concurrentPerform(iterations: blocks.count) { index in // FIXME: this crashes sometimes -// blocks[index]() -// } -// -// // Two readers, one writer -// XCTAssertEqual(totalOpenConnectionCount, 3) -// -// // Writer is still open -// XCTAssertEqual(openConnectionCount, 1) -// } + func test_DatabasePool_releaseMemory_closes_reader_connections() throws { + // A complicated test setup that opens multiple reader connections. + let countQueue = DispatchQueue(label: "GRDB") + var openConnectionCount = 0 + var totalOpenConnectionCount = 0 + + dbConfiguration.SQLiteConnectionDidOpen = { + countQueue.sync { + totalOpenConnectionCount += 1 + openConnectionCount += 1 + } + } + + dbConfiguration.SQLiteConnectionDidClose = { + countQueue.sync { + openConnectionCount -= 1 + } + } + + let dbPool = try makeDatabasePool() + try dbPool.write { db in + try db.execute(sql: "CREATE TABLE items (id INTEGER PRIMARY KEY)") + for _ in 0..<2 { + try db.execute(sql: "INSERT INTO items (id) VALUES (NULL)") + } + } + + // Block 1 Block 2 Block3 + // SELECT * FROM items + // step + // > + let s1 = DispatchSemaphore(value: 0) + // SELECT * FROM items + // step + // > + let s2 = DispatchSemaphore(value: 0) + // step step + // > + let s3 = DispatchSemaphore(value: 0) + // end end releaseMemory + + let block1 = { () in + try! dbPool.read { db in + let cursor = try Row.fetchCursor(db, sql: "SELECT * FROM items") + XCTAssertTrue(try cursor.next() != nil) + s1.signal() + _ = s2.wait(timeout: .distantFuture) + XCTAssertTrue(try cursor.next() != nil) + s3.signal() + XCTAssertTrue(try cursor.next() == nil) + } + } + let block2 = { () in + _ = s1.wait(timeout: .distantFuture) + try! dbPool.read { db in + let cursor = try Row.fetchCursor(db, sql: "SELECT * FROM items") + XCTAssertTrue(try cursor.next() != nil) + s2.signal() + XCTAssertTrue(try cursor.next() != nil) + XCTAssertTrue(try cursor.next() == nil) + } + } + let block3 = { () in + _ = s3.wait(timeout: .distantFuture) + dbPool.releaseMemory() + } + let blocks = [block1, block2, block3] + DispatchQueue.concurrentPerform(iterations: blocks.count) { index in // FIXME: this crashes sometimes + blocks[index]() + } + + // Two readers, one writer + XCTAssertEqual(totalOpenConnectionCount, 3) + + // Writer is still open + XCTAssertEqual(openConnectionCount, 1) + } + + func test_DatabasePool_releaseMemory_closes_reader_connections_when_persistentReaderConnections_is_false() throws { + var persistentConnectionCount = 0 + + dbConfiguration.SQLiteConnectionDidOpen = { + persistentConnectionCount += 1 + } + + dbConfiguration.SQLiteConnectionDidClose = { + persistentConnectionCount -= 1 + } + + dbConfiguration.persistentReaderConnections = false + + let dbPool = try makeDatabasePool() + XCTAssertEqual(persistentConnectionCount, 1) // writer + + try dbPool.read { _ in } + XCTAssertEqual(persistentConnectionCount, 2) // writer + reader + + dbPool.releaseMemory() + XCTAssertEqual(persistentConnectionCount, 1) // writer + } + + func test_DatabasePool_releaseMemory_does_not_close_reader_connections_when_persistentReaderConnections_is_true() throws { + var persistentConnectionCount = 0 + + dbConfiguration.SQLiteConnectionDidOpen = { + persistentConnectionCount += 1 + } + + dbConfiguration.SQLiteConnectionDidClose = { + persistentConnectionCount -= 1 + } + + dbConfiguration.persistentReaderConnections = true + + let dbPool = try makeDatabasePool() + XCTAssertEqual(persistentConnectionCount, 1) // writer + + try dbPool.read { _ in } + XCTAssertEqual(persistentConnectionCount, 2) // writer + reader + + dbPool.releaseMemory() + XCTAssertEqual(persistentConnectionCount, 2) // writer + reader + } func testBlocksRetainConnection() throws { let countQueue = DispatchQueue(label: "GRDB") From 6ca4986bc2ba985d5b61a4359aa64731a6bca9f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Rou=C3=A9?= Date: Fri, 17 Mar 2023 13:51:29 +0100 Subject: [PATCH 07/11] CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff9744e3a8..e00294c10a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -109,7 +109,7 @@ GRDB adheres to [Semantic Versioning](https://semver.org/), with one exception: ## Next Release -- **New**: [#1350](https://github.com/groue/GRDB.swift/pull/1350) by [@groue](https://github.com/groue): ValueObservation on DatabasePool no longer opens a new database connection when it starts +- **New**: [#1350](https://github.com/groue/GRDB.swift/pull/1350) by [@groue](https://github.com/groue): DatabasePool won't close read-only connections if requested, and ValueObservation no longer opens a new database connection when it starts. ## 6.9.2 From 449c243a361c0e66213bb4038614eb9d2afdcae6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Sat, 18 Mar 2023 11:08:56 +0100 Subject: [PATCH 08/11] Rename persistentReaderConnections to dbConfiguration.persistentReadOnlyConnections --- GRDB/Core/Configuration.swift | 30 ++++++++----------- GRDB/Core/DatabasePool.swift | 16 +++++----- .../Extension/Configuration.md | 2 +- .../DatabasePoolReleaseMemoryTests.swift | 8 ++--- 4 files changed, 25 insertions(+), 31 deletions(-) diff --git a/GRDB/Core/Configuration.swift b/GRDB/Core/Configuration.swift index f2868d108d..495b326909 100644 --- a/GRDB/Core/Configuration.swift +++ b/GRDB/Core/Configuration.swift @@ -374,25 +374,19 @@ public struct Configuration { #endif /// A boolean value indicating whether read-only connections should be - /// kept open as long as they remain in a valid state. + /// kept open. /// - /// This configuration applies to ``DatabasePool`` only. The default value - /// is false. - /// - /// A `DatabasePool` automatically closes read-only connections on - /// various occasions, in order to spare memory - /// (see ``DatabasePool/releaseMemory()``), or when connections enter an - /// invalid state. - /// - /// When this flag is true, only invalid connections are automatically - /// closed. Valid connections, once opened, are kept alive until the - /// `DatabasePool` is deinitialized, or one of those methods is called: - /// ``DatabaseReader/close()``, - /// ``DatabasePool/invalidateReadOnlyConnections()``. - /// - /// Consider using this flag when profiling your application reveals - /// that a lot of time is spent opening new SQLite connections. - public var persistentReaderConnections = false + /// This configuration flag applies to ``DatabasePool`` only. The + /// default value is false. + /// + /// When the flag is false, a `DatabasePool` closes read-only + /// connections when requested to dispose non-essential memory with + /// ``DatabasePool/releaseMemory()``. When true, those connections are + /// kept open. + /// + /// Consider setting this flag to true when profiling your application + /// reveals that a lot of time is spent opening new SQLite connections. + public var persistentReadOnlyConnections = false // MARK: - Factory Configuration diff --git a/GRDB/Core/DatabasePool.swift b/GRDB/Core/DatabasePool.swift index b08a139061..a19baf9185 100644 --- a/GRDB/Core/DatabasePool.swift +++ b/GRDB/Core/DatabasePool.swift @@ -173,8 +173,8 @@ extension DatabasePool { /// This method is synchronous, and blocks the current thread until all /// database accesses are completed. /// - /// This method closes all reader connections, unless the - /// ``Configuration/persistentReaderConnections`` configuration flag + /// This method closes all read-only connections, unless the + /// ``Configuration/persistentReadOnlyConnections`` configuration flag /// is set. /// /// - warning: This method can prevent concurrent reads from executing, @@ -184,7 +184,7 @@ extension DatabasePool { // Release writer memory writer.sync { $0.releaseMemory() } - if configuration.persistentReaderConnections { + if configuration.persistentReadOnlyConnections { // Keep existing readers readerPool?.forEach { reader in reader.sync { $0.releaseMemory() } @@ -207,15 +207,15 @@ extension DatabasePool { /// Eventually frees as much memory as possible, by disposing /// non-essential memory. /// - /// This method eventually closes all reader connections, unless the - /// ``Configuration/persistentReaderConnections`` configuration flag + /// This method eventually closes all read-only connections, unless the + /// ``Configuration/persistentReadOnlyConnections`` configuration flag /// is set. /// /// Unlike ``releaseMemory()``, this method does not prevent concurrent /// database accesses when it is executing. But it does not notify when /// non-essential memory has been freed. public func releaseMemoryEventually() { - if configuration.persistentReaderConnections { + if configuration.persistentReadOnlyConnections { // Keep existing readers readerPool?.forEach { reader in reader.async { $0.releaseMemory() } @@ -627,11 +627,11 @@ extension DatabasePool: DatabaseReader { /// After this method is called, read-only database access methods will use /// new SQLite connections. /// - /// Eventual concurrent read-only accesses are not invalidated: they will + /// Eventual concurrent read-only accesses are not interrupted, and /// proceed until completion. /// /// - This method closes all read-only connections, even if the - /// ``Configuration/persistentReaderConnections`` configuration flag + /// ``Configuration/persistentReadOnlyConnections`` configuration flag /// is set. public func invalidateReadOnlyConnections() { readerPool?.removeAll() diff --git a/GRDB/Documentation.docc/Extension/Configuration.md b/GRDB/Documentation.docc/Extension/Configuration.md index aaf64e58fd..e2e4b2743c 100644 --- a/GRDB/Documentation.docc/Extension/Configuration.md +++ b/GRDB/Documentation.docc/Extension/Configuration.md @@ -92,7 +92,7 @@ do { - ``label`` - ``maximumReaderCount`` - ``observesSuspensionNotifications`` -- ``persistentReaderConnections`` +- ``persistentReadOnlyConnections`` - ``prepareDatabase(_:)`` - ``publicStatementArguments`` - ``transactionClock`` diff --git a/Tests/GRDBTests/DatabasePoolReleaseMemoryTests.swift b/Tests/GRDBTests/DatabasePoolReleaseMemoryTests.swift index 31e26faa8f..9b6bb306c4 100644 --- a/Tests/GRDBTests/DatabasePoolReleaseMemoryTests.swift +++ b/Tests/GRDBTests/DatabasePoolReleaseMemoryTests.swift @@ -198,7 +198,7 @@ class DatabasePoolReleaseMemoryTests: GRDBTestCase { XCTAssertEqual(openConnectionCount, 1) } - func test_DatabasePool_releaseMemory_closes_reader_connections_when_persistentReaderConnections_is_false() throws { + func test_DatabasePool_releaseMemory_closes_reader_connections_when_persistentReadOnlyConnections_is_false() throws { var persistentConnectionCount = 0 dbConfiguration.SQLiteConnectionDidOpen = { @@ -209,7 +209,7 @@ class DatabasePoolReleaseMemoryTests: GRDBTestCase { persistentConnectionCount -= 1 } - dbConfiguration.persistentReaderConnections = false + dbConfiguration.persistentReadOnlyConnections = false let dbPool = try makeDatabasePool() XCTAssertEqual(persistentConnectionCount, 1) // writer @@ -221,7 +221,7 @@ class DatabasePoolReleaseMemoryTests: GRDBTestCase { XCTAssertEqual(persistentConnectionCount, 1) // writer } - func test_DatabasePool_releaseMemory_does_not_close_reader_connections_when_persistentReaderConnections_is_true() throws { + func test_DatabasePool_releaseMemory_does_not_close_reader_connections_when_persistentReadOnlyConnections_is_true() throws { var persistentConnectionCount = 0 dbConfiguration.SQLiteConnectionDidOpen = { @@ -232,7 +232,7 @@ class DatabasePoolReleaseMemoryTests: GRDBTestCase { persistentConnectionCount -= 1 } - dbConfiguration.persistentReaderConnections = true + dbConfiguration.persistentReadOnlyConnections = true let dbPool = try makeDatabasePool() XCTAssertEqual(persistentConnectionCount, 1) // writer From 5d9b9edfe168099048481733769a97b850e08235 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Sun, 19 Mar 2023 12:39:48 +0100 Subject: [PATCH 09/11] Fix asyncWALSnapshotTransaction usage. We must use its result asynchronously. --- GRDB/Core/DatabasePool.swift | 3 + GRDB/Core/SerializedDatabase.swift | 15 ++++ GRDB/Core/WALSnapshotTransaction.swift | 13 ++- .../Observers/ValueConcurrentObserver.swift | 90 ++++++++++--------- 4 files changed, 77 insertions(+), 44 deletions(-) diff --git a/GRDB/Core/DatabasePool.swift b/GRDB/Core/DatabasePool.swift index a19baf9185..b89f0c4c1f 100644 --- a/GRDB/Core/DatabasePool.swift +++ b/GRDB/Core/DatabasePool.swift @@ -683,6 +683,9 @@ extension DatabasePool: DatabaseReader { } /// Returns a long-lived WAL snapshot transaction on a reader connection. + /// + /// - important: The `completion` argument is executed in a serial + /// dispatch queue, so make sure you use the transaction asynchronously. func asyncWALSnapshotTransaction(_ completion: @escaping (Result) -> Void) { guard let readerPool else { completion(.failure(DatabaseError.connectionIsClosed())) diff --git a/GRDB/Core/SerializedDatabase.swift b/GRDB/Core/SerializedDatabase.swift index 482a34978c..d7c4d4fbf6 100644 --- a/GRDB/Core/SerializedDatabase.swift +++ b/GRDB/Core/SerializedDatabase.swift @@ -140,6 +140,21 @@ final class SerializedDatabase { } } + /// Executes database operations, returns their result after they have + /// finished executing, and allows or forbids long-lived transactions. + /// + /// This method is not reentrant. + /// + /// - parameter allowingLongLivedTransaction: When true, the + /// ``Configuration/allowsUnsafeTransactions`` configuration flag is + /// ignored until this method is called again with false. + func reentrantSync(allowingLongLivedTransaction: Bool, _ body: (Database) throws -> T) rethrows -> T { + try reentrantSync { db in + self.allowsUnsafeTransactions = allowingLongLivedTransaction + return try body(db) + } + } + /// Executes database operations, and returns their result after they /// have finished executing. /// diff --git a/GRDB/Core/WALSnapshotTransaction.swift b/GRDB/Core/WALSnapshotTransaction.swift index fb0323cec5..32eb3afca1 100644 --- a/GRDB/Core/WALSnapshotTransaction.swift +++ b/GRDB/Core/WALSnapshotTransaction.swift @@ -65,15 +65,24 @@ class WALSnapshotTransaction { /// Executes database operations in the snapshot transaction, and /// returns their result after they have finished executing. func read(_ value: (Database) throws -> T) rethrows -> T { - // TODO: we should check the validity of the snapshot, as DatabaseSnapshotPool does. + // We should check the validity of the snapshot, as DatabaseSnapshotPool does. try reader.sync(value) } + /// Schedules database operations for execution, and + /// returns immediately. + func asyncRead(_ value: @escaping (Database) -> Void) { + // We should check the validity of the snapshot, as DatabaseSnapshotPool does. + reader.async(value) + } + private static func commitAndRelease( reader: SerializedDatabase, release: (_ isInsideTransaction: Bool) -> Void) { - let isInsideTransaction = reader.sync(allowingLongLivedTransaction: false) { db in + // WALSnapshotTransaction may be deinitialized in the dispatch + // queue of its reader: allow reentrancy. + let isInsideTransaction = reader.reentrantSync(allowingLongLivedTransaction: false) { db in try? db.commit() return db.isInsideTransaction } diff --git a/GRDB/ValueObservation/Observers/ValueConcurrentObserver.swift b/GRDB/ValueObservation/Observers/ValueConcurrentObserver.swift index 7211c20c0a..27a4184e86 100644 --- a/GRDB/ValueObservation/Observers/ValueConcurrentObserver.swift +++ b/GRDB/ValueObservation/Observers/ValueConcurrentObserver.swift @@ -324,50 +324,56 @@ extension ValueConcurrentObserver { do { // Fetch let walSnapshotTransaction = try result.get() - try walSnapshotTransaction.read { db in - let fetchedValue: Reducer.Fetched - let initialRegion: DatabaseRegion - - switch self.trackingMode { - case let .constantRegion(regions): - fetchedValue = try databaseAccess.fetch(db) - let region = try DatabaseRegion.union(regions)(db) - initialRegion = try region.observableRegion(db) + // Second async jump because that's how + // `DatabasePool.asyncWALSnapshotTransaction` has to be used. + walSnapshotTransaction.asyncRead { db in + do { + let fetchedValue: Reducer.Fetched + let initialRegion: DatabaseRegion - case .constantRegionRecordedFromSelection, - .nonConstantRegionRecordedFromSelection: - (fetchedValue, initialRegion) = try databaseAccess.fetchRecordingObservedRegion(db) - } - - // Reduce - // - // Reducing is performed asynchronously, so that we do not lock - // a database dispatch queue longer than necessary. - self.reduceQueue.async { - let isNotifying = self.lock.synchronized { self.notificationCallbacks != nil } - guard isNotifying else { return /* Cancelled */ } + switch self.trackingMode { + case let .constantRegion(regions): + fetchedValue = try databaseAccess.fetch(db) + let region = try DatabaseRegion.union(regions)(db) + initialRegion = try region.observableRegion(db) + + case .constantRegionRecordedFromSelection, + .nonConstantRegionRecordedFromSelection: + (fetchedValue, initialRegion) = try databaseAccess.fetchRecordingObservedRegion(db) + } - do { - guard let initialValue = try self.reducer._value(fetchedValue) else { - fatalError("Broken contract: reducer has no initial value") - } + // Reduce + // + // Reducing is performed asynchronously, so that we do not lock + // a database dispatch queue longer than necessary. + self.reduceQueue.async { + let isNotifying = self.lock.synchronized { self.notificationCallbacks != nil } + guard isNotifying else { return /* Cancelled */ } - // Notify - self.scheduler.schedule { - let onChange = self.lock.synchronized { self.notificationCallbacks?.onChange } - guard let onChange else { return /* Cancelled */ } - onChange(initialValue) + do { + guard let initialValue = try self.reducer._value(fetchedValue) else { + fatalError("Broken contract: reducer has no initial value") + } + + // Notify + self.scheduler.schedule { + let onChange = self.lock.synchronized { self.notificationCallbacks?.onChange } + guard let onChange else { return /* Cancelled */ } + onChange(initialValue) + } + } catch { + self.notifyError(error) } - } catch { - self.notifyError(error) } + + // Start observation + self.asyncStartObservation( + from: databaseAccess, + walSnapshotTransaction: walSnapshotTransaction, + initialRegion: initialRegion) + } catch { + self.notifyError(error) } - - // Start observation - self.asyncStartObservation( - from: databaseAccess, - walSnapshotTransaction: walSnapshotTransaction, - initialRegion: initialRegion) } } catch { self.notifyError(error) @@ -389,10 +395,10 @@ extension ValueConcurrentObserver { // Transaction is needed for comparing version snapshots try writerDB.isolated(readOnly: true) { - // Keep walSnapshotTransaction alive until we have compared - // database versions. It prevents database checkpointing, - // and keeps WAL snapshots (`sqlite3_snapshot`) valid - // and comparable. + // Keep walSnapshotTransaction alive until we have + // compared database versions. `WALSnapshot` alone is + // not able to prevent database checkpointing, and keep + // snapshots comparable. let isModified = withExtendedLifetime(walSnapshotTransaction) { guard let currentWALSnapshot = try? WALSnapshot(writerDB) else { return true From 5629cfe6ffaba1ac45aa6413fcef9f4241f33845 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Sun, 19 Mar 2023 13:56:56 +0100 Subject: [PATCH 10/11] Fix documentation --- GRDB/Core/Configuration.swift | 11 +++++++---- GRDB/Core/SerializedDatabase.swift | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/GRDB/Core/Configuration.swift b/GRDB/Core/Configuration.swift index 495b326909..58889b4bb6 100644 --- a/GRDB/Core/Configuration.swift +++ b/GRDB/Core/Configuration.swift @@ -301,13 +301,15 @@ public struct Configuration { /// If nil, GRDB picks a default one. var readonlyBusyMode: Database.BusyMode? = nil - /// The maximum number of concurrent read-only connections. + /// The maximum number of concurrent reader connections. /// - /// This configuration applies to ``DatabasePool`` only. The default value - /// is 5. + /// This configuration has effect on ``DatabasePool`` and + /// ``DatabaseSnapshotPool`` only. The default value is 5. /// /// You can query this value at runtime in order to get the actual capacity - /// for concurrent reads of any ``DatabaseReader``. For example: + /// for concurrent reads of any ``DatabaseReader``. In this context, + /// ``DatabaseQueue`` and ``DatabaseSnapshot`` have a capacity of 1, + /// because they can't perform two concurrent reads. For example: /// /// ```swift /// var config = Configuration() @@ -321,6 +323,7 @@ public struct Configuration { /// print(dbQueue.configuration.maximumReaderCount) // 1 /// print(dbPool.configuration.maximumReaderCount) // 5 /// print(dbSnapshot.configuration.maximumReaderCount) // 1 + /// ``` public var maximumReaderCount: Int = 5 /// The quality of service of database accesses. diff --git a/GRDB/Core/SerializedDatabase.swift b/GRDB/Core/SerializedDatabase.swift index d7c4d4fbf6..8861deb9ca 100644 --- a/GRDB/Core/SerializedDatabase.swift +++ b/GRDB/Core/SerializedDatabase.swift @@ -222,7 +222,7 @@ final class SerializedDatabase { } } - // Schedules database operations for execution, and returns immediately. + /// Schedules database operations for execution, and returns immediately. func async(_ block: @escaping (Database) -> Void) { queue.async { block(self.db) From 600d1b84b6daae23283d31581383dfbe338bead3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Rou=C3=A9?= Date: Mon, 20 Mar 2023 07:02:00 +0100 Subject: [PATCH 11/11] Update GRDB/Core/SerializedDatabase.swift Co-authored-by: Rogerio de Paula Assis --- GRDB/Core/SerializedDatabase.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GRDB/Core/SerializedDatabase.swift b/GRDB/Core/SerializedDatabase.swift index 8861deb9ca..f1dfe05721 100644 --- a/GRDB/Core/SerializedDatabase.swift +++ b/GRDB/Core/SerializedDatabase.swift @@ -143,7 +143,7 @@ final class SerializedDatabase { /// Executes database operations, returns their result after they have /// finished executing, and allows or forbids long-lived transactions. /// - /// This method is not reentrant. + /// This method is reentrant. /// /// - parameter allowingLongLivedTransaction: When true, the /// ``Configuration/allowsUnsafeTransactions`` configuration flag is