-
Notifications
You must be signed in to change notification settings - Fork 6
[WIP] GRDB Support #78
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
base: main
Are you sure you want to change the base?
Conversation
func __leaseAll(callback: @escaping (Any, [Any]) -> Void) async throws { | ||
// TODO, actually use all connections | ||
do { | ||
try await pool.write { pointer in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @stevensJourney - it looks like you're about to need a way to iterate all available connections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!, yes, in order to completely satisfy our internal driver requirements, we would require this.
I haven't full scanned through the GRDB docs yet, but I haven't seen a way yet to achieve this. Is this currently possible with GRDB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not yet, that would be a new feature. We'd have to clarify the exact required semantics.
In particular, I wonder if the abstract pool you have to conform to is assumed to run a fixed set of available connections, or if it is allowed to close existing connections and open new ones.
- If it is assumed to run a fixed set of available connections, then
leaseAll
makes sure that all future database access are impacted by the effects of the callback. - If the pool can close and open connections at will, then it is possible for a future database access to run in a connection that did not run the callback.
To lift the ambiguity, the semantics of leaseAll
need to be clarified.
There is another related clarification that is needed: can concurrent database accesses run during the execution of leaseAll
, or not?
Maybe there are other constraints that I'm not aware of. In all cases, make sure you make the semantics crystal clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, GRDB has a prepareDatabase
callback that executes code in any connection that opens, before it is made available to the rest of the application. This is where GRDB users register functions, collations, and make general connection setup:
var config = Configuration()
config.prepareDatabase { db in
// Setup stuff on demand
}
let dbPool = try DatabasePool(path: "...", configuration.config)
try dbPool.read { db in // or dbPool.write
// Setup has run on that connection, guaranteed
}
If the only goal of leaseAll
is to execute database code early, then I would suggest replacing it with a similar pattern in the PowerSync pool protocol. This should be possible in all target languages that have a notion of closure that can be executed later. And this would void all the complex semantics questions I have asked above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, if the goal of leaseAll
is to call sqlite3_db_release_memory()
, then DatabasePool
has a ready-made releaseMemory()
method.
In summary, I strongly suggest clarifying the intent behind leaseAll
, so that we avoid the XY problem (and leaseAll
has a big XY smell).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed responses and queries.
The current implementation of leaseAll
in this SDK assumes a lock is taken on all the current SQLite connections. The pool can be fixed or dynamically sized - the only requirement is that the lock be taken at the time of and during the request.
prepareDatabase
is a good solution for executing code when connections are opened, but this does not align with the current requirements of leaseAll
.
You are very correct about the XY Problem scenario described. For context, we allow users to update the PowerSync schema after the client has been initialised. E.g. This is triggered here, We currently apply the change using a write connection and thereafter refresh the schema on the read connections - preventing any reads during this operation: which might be invalid.
If are any alternatives for ensuring the Schema is refreshed on read connections, I think we could avoid requiring this functionality for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks for the clarification 👍 When you're on the leaseAll implementation, please open a new discussion in the GRDB repo. All the building blocks are there, we just need to design a public API.
) { | ||
// Register the PowerSync core extension | ||
config.prepareDatabase { database in | ||
guard let bundle = Bundle(identifier: "co.powersync.sqlitecore") else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think on watchOS we would have to call the powersync_init_static()
C function instead, since everything is linked statically there.
} | ||
|
||
func databaseDidChange(with event: DatabaseEvent) { | ||
onChange(event.tableName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we should potentially buffer those until databaseDidCommit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently buffered in GRDBConnectionPool
and the Kotlin adapter SwiftSQLiteConnectionPoolAdapter
. I haven't checked this thoroughly yet, but I was hesitant to use commit hooks for the buffering - since IIRC the C commit hooks fire before the commit actually completed.
GRDB does seem to distinguish between databaseDidCommit
and databaseWillCommit
, so it could be an improvement to use databaseDidCommit
Overview
This builds off the amazing work of powersync-ja/powersync-kotlin#230. The linked PR adds support for creating a
PowerSyncDatabase
by supplying a customSQLiteConnectionPool
which manages SQLite connections.Some small additions and wip tests were added to the Kotlin implementation here
This PR adds Swift bindings to the Kotlin PR and then adds a
PowerSyncGRDB
Product to our SPM package. This product adapts a GRDB DatabasePool to aSQLiteConnectionPool
accepted by the PowerSync SDK.This makes use of the fact that GRDB exposes the raw C SQLite pointer for it's connections. We then use this pointer with the newly introduced native
Database
kotlin implementation.The
PowerSyncGRDB
product provides a means to generate a GRDBConfiguration
. This configuration is used when creating the GRDBDatabasePool
. We supply implementation for the configuration to load our PowerSync Rust core extension.APIs are still a WIP
Consumers can create a Pool with:
Consumers can then pass this pool when creating the PowerSyncDatabase.
Using the
DatabasePool
in the PowerSync SDK results in the same locking mechanisms being used between instances of thePowerSyncDatabase
andDatabasePool
. Consumers should be safe to alternate between both clients.TODOs: