-
-
Notifications
You must be signed in to change notification settings - Fork 731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SQLInterface for dynamic replacement of SQLite implementations #1701
base: development
Are you sure you want to change the base?
Conversation
Hello @marcprux, Wow, that's ambitious :-) This is a much welcomed exploration. Custom SQLite builds and SQLCipher indeed require some uneasy "gymnastics". There are two important GRDB features that this pull request does not address, and I'm wondering what could be done in order to fulfill the needs of GRDB users:
|
…es to control whether to use the new GRDBSQLiteDynamic module and enable performance tests
I've rigged up the I've run these performance tests a few times, and the result is that using
If this isn't a tolerable degradation, one option could be to add another package product, like
I think this would translate directly, since we could just add the same
I suppose these checks would need to turn into dynamic feature availability checks. So instead of: #if SQLITE_ENABLE_FTS5
…
#endif we would do something like: if SQLI.hasFeature(.fts5) {
}
public enum SystemSQLiteInterface : SQLiteInterface {
public func hasFeature(_ feature: SQLiteInterface.Feature) -> Bool {
if feature == .snapshots { return false }
return true
}
public func sqlite3_snapshot_recover(_ db: OpaquePointer!, _ zDb: UnsafePointer<CChar>!) -> Int32 {
fatalError("unsupported in this build")
}
}
Do you mean for non-standard API additions in custom builds, like sqlite3_rekey? In these scenarios, we could assume that the customization has some standard mechanism for achieving the same functionality (like Happy to explore this further and sketch out of how these availability checks might look. |
Thanks for running the tests!
A 8% regression is more than I expected, for an I/O library... Anyway. GRDB targets SQLite, not a remote database server: we can't just assume network swallows everything. Performance counts. It's one of the GRDB selling points. We should aim at improving performances, instead of degrading them. If I'm correct and the existential is the source of the regression, then database connections would need to become generic in order to enable compiler specialization. Performance is important, and source compatibility is important as well. I guess we'd need to typealias Not easy problems to solve.
It is not a corner case. It is the main use case. Why do people use a custom SQLite build, if not for a total control on the enabled SQLite features? Availability checks must absolutely be avoided. This is not trivial:
You can check for the availability of SQL features at runtime (those are just strings), but you can't check for the availability of
And It can probably be achieved with a rich set of protocols instead of the single
That's very nice of you. Take care that I have exposed some serious issues to solve. Maybe you are on a good track, but maybe this is a dead-end as well. I do not want that you think you're spending time in vain, and that's why I have tried to expose as clearly as possible some design goals above. |
Agreed, I was a bit surprised as well. I haven't delved into the contents of the performance tests and whether they exemplify real-world use, but regardless I agree that it is a significant degradation.
Not only that, but any type that references any However, one other option could be to instead have two separate GRDB targets (and associated products) with the same sources, but one that links directly to the system
I'll ponder this issue some more, but it sounds like it is all moot if we can't find a solution to the performance implications of this change. |
I was actually contemplating this solution as acceptable. You keep on:
I wish we could avoid this performance reduction. Separate targets are not a bad idea, though. They could help choosing between system and custom SQLite builds, so that only the desired SQLite version would be linked. It's very complicated to deal with duplicated symbols. SQLite build system does not support symbol prefixing (maybe Clang can do it (
I think a forest of protocols that encode the presence of SQLite features could help. I'll think about it a little more and come back. |
We could probably encode SQLite interfaces in a set of protocols: // Encodes the minimal supported SQLite version
public protocol SQLiteInterfaceMinimal {
func sqlite3_libversion_number() -> CInt
}
// Encode SQLite versions
public protocol SQLiteInterface_3035000 {
// APIs enabled from SQLite 3.35.0
}
public protocol SQLiteInterface_3037000 {
// APIs enabled from SQLite 3.37.0
}
// Encode compilation options
public protocol SQLiteInterface_3035000_HAS_CODEC {
// APIs enabled from SQLite 3.35.0 with SQLITE_HAS_CODEC
}
public protocol SQLiteInterface_3035000_ENABLE_SNAPSHOT {
// APIs enabled from SQLite 3.35.0 with SQLITE_ENABLE_SNAPSHOT
}
public protocol SQLiteInterface_3037000_ENABLE_SNAPSHOT {
// APIs enabled from SQLite 3.37.0 with SQLITE_ENABLE_SNAPSHOT
} System InterfaceThe system interface could be defined as: import SQLite3
public struct SystemSQLiteInterface: SQLiteInterfaceMinimal {
@inlinable @_transparent @_alwaysEmitIntoClient @inline(__always)
func sqlite3_libversion_number() -> CInt {
SQLite3.sqlite3_libversion_number()
}
}
@available(iOS 14.0, *) // made up numbers!
extension SystemSQLiteInterface: SQLiteInterface_3035000 { }
@available(iOS 14.0, *) // made up numbers!
extension SystemSQLiteInterface: SQLiteInterface_3035000_ENABLE_SNAPSHOT { }
@available(iOS 15.0, *) // made up numbers!
extension SystemSQLiteInterface: SQLiteInterface_3037000 { }
@available(iOS 15.0, *) // made up numbers!
extension SystemSQLiteInterface: SQLiteInterface_3037000_ENABLE_SNAPSHOT { }
// Typealiases for source compatibility
public typealias Database = SQLiteConnection<SystemSQLiteInterface>
public typealias DatabaseQueue = SQLiteDatabaseQueue<SystemSQLiteInterface>
public typealias DatabasePool = SQLiteDatabasePool<SystemSQLiteInterface> (Note all annotations on SQLCipherThis module would ship with a pre-build SQLCipher amalgamation, with a fixed set of SQLite compilation options. Users could fork and tweaks compilation options. Hence the import SQLCipher
public struct SQLCipherInterface: SQLiteInterfaceMinimal {
@inlinable @_transparent @_alwaysEmitIntoClient @inline(__always)
func sqlite3_libversion_number() -> CInt {
SQLCipher.sqlite3_libversion_number()
}
}
extension SQLCipherInterface: SQLiteInterface_3035000 { }
extension SQLCipherInterface: SQLiteInterface_3035000_HAS_CODEC { }
extension SQLCipherInterface: SQLiteInterface_3037000 { }
#if SQLITE_ENABLE_SNAPSHOT // customizable by Package.swift
extension SQLCipherInterface: SQLiteInterface_3035000_ENABLE_SNAPSHOT { }
extension SQLCipherInterface: SQLiteInterface_3037000_ENABLE_SNAPSHOT { }
#endif
// Typealiases for source compatibility
public typealias Database = SQLiteConnection<SQLCipherInterface>
public typealias DatabaseQueue = SQLiteDatabaseQueue<SQLCipherInterface>
public typealias DatabasePool = SQLiteDatabasePool<SQLCipherInterface> Custom SQLiteThis module would ship with a pre-build SQLite amalgamation, with a fixed set of SQLite compilation options. Users could fork and tweaks compilation options. Hence the import CustomSQLite
public struct CustomSQLiteInterface: SQLiteInterfaceMinimal {
@inlinable @_transparent @_alwaysEmitIntoClient @inline(__always)
func sqlite3_libversion_number() -> CInt {
CustomSQLite.sqlite3_libversion_number()
}
}
extension CustomSQLiteInterface: SQLiteInterface_3035000 { }
extension CustomSQLiteInterface: SQLiteInterface_3037000 { }
#if SQLITE_ENABLE_SNAPSHOT // customizable by Package.swift
extension CustomSQLiteInterface: SQLiteInterface_3035000_ENABLE_SNAPSHOT { }
extension CustomSQLiteInterface: SQLiteInterface_3037000_ENABLE_SNAPSHOT { }
#endif
// Typealiases for source compatibility
public typealias Database = SQLiteConnection<CustomSQLiteInterface>
public typealias DatabaseQueue = SQLiteDatabaseQueue<CustomSQLiteInterface>
public typealias DatabasePool = SQLiteDatabasePool<CustomSQLiteInterface> |
I've updated the PR by getting rid of the global public protocol SQLiteAPI {
associatedtype SQLI: SQLiteInterface
} The new SQLiteInterfaceSupport.swift file provides retroactive conformance for most of the types that call any extension Database : SQLiteAPI { public typealias SQLI = DefaultSQLiteInterface }
extension Row : SQLiteAPI { public typealias SQLI = DefaultSQLiteInterface }
public enum SystemSQLiteInterface : SQLiteInterface {
case system
public static var SQLITE_VERSION: String { SQLite3.SQLITE_VERSION }
public static var SQLITE_VERSION_NUMBER: Int32 { SQLite3.SQLITE_VERSION_NUMBER }
public static func sqlite3_libversion() -> UnsafePointer<CChar>! { SQLite3.sqlite3_libversion() }
public static func sqlite3_libversion_number() -> Int32 { SQLite3.sqlite3_libversion_number() }
// et cetera
} This enables us to experiment without being too high-touch (although I did need to make a bunch of changes to some of the GRDB types). The type extensions in SQLiteInterfaceSupport.swift are the primary ones that would need to become generified. Plus, the foundation types in there that implement Performance can still be compared between the old and new with:
(I'm unable to provide good performance comparisons just now because my machine is loaded down with other processes, but you should be able to run those commands against a checkout of this PR to see for yourself.) Some notes:
Let me know if you think this direction is worth pursuing. |
I'm really interested in those explorations, and at the same time:
Breaking changes are probably unavoidable, and there are many details to be ironed out before an eventual merge. So this will probably not ship in GRDB 7. I honestly don't know what to suggest to you, because I respect your time very much. I understand you want to use GRDB on Android, and maybe you can do it from your fork already. That would already be a great achievement. And you'd get a lot of experience from it. Experience that could be shared here, for the benefit of all. By limiting your fork to your own needs, you can ignore features that you do not use, such as FTS5. I recommend supporting SQLite snapshots if you want to use In all cases, this conversation is precious: it's the first time I start to believe that abstracting the SQLite implementation is possible. I fully agree about the "gymnastics" your have described. Since the vanishing of CocoaPods, supporting SQLCipher has become complicated. Supporting custom SQLite builds has always been complicated. Making the life of GRDB users better is a continuous effort. Some features take time, and I'm OK with this (if I weren't, all pleasure maintaining this library would vanish). |
I've scaled back my ambitions and I think I've found a good compromise in #1708. This would make GRDB more easily accommodate a |
This draft PR replaces the
GRDBSQLite
systemLibrary with a target that declares all the SQLite3 symbols inSQLiteThunk.swift
, which delegate all their implementations through a globalpublic var SQLI: SQLiteInterface
protocol, which itself declares the functions and variables for the entire SQLite3 interface. There is also aSystemSQLiteInterface.swift
implementation that implements the interface and passes them all through to the systemSQLite3
module.The idea is that by enabling the dynamic replacement of the SQLite3 implementation, a custom build of SQLite can be included in a way that is compatible with pure SwiftPM, and doesn't require any of the special build gymnastics that are currently needed. This would enable, for example, swapping in a custom SQLCipher build build at runtime. My personal motivation is to bring GRDB to Android, which doesn't expose any user-accessible SQLite library and so always needs to include a SQLite source build.
This draft PR is minimally code-intrusive in order to illustrate the salient points, but one might imagine that rather than having a single global
public var SQLI: SQLiteInterface
variable, theSQLiteInterface
could be provided at GRDB initialization time, which opens the possibility of having multiple simultaneous SQLite builds in the same app. But I imagine most environments will only care about a single global implementation, so I doubt it would be worth the trouble.Two rough spots right now are that I had to disable snapshot and FTS5 support, since they both need to use custom structs from the system
SQLite3
library, and I didn't bother creating pass-though wrappers for these. I expect that resolving this would be a straightforward exercise. With these tests disabled, all the tests pass on my machine:Pull Request Checklist
development
branch.make smokeTest
terminal command runs without failure.