-
-
Notifications
You must be signed in to change notification settings - Fork 813
DatabasePool won't close read-only connections if requested, and ValueObservation no longer opens a new database connection when it starts #1350
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
Conversation
|
Note to self: it could be useful for users to pre-heat reader connections, and also to prevent connections from being closed when the app reclaims memory (automatic on iOS apps when they enter background or receive a memory warning). TBD in another PR. EDIT: addressed with the new |
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.
…ion, 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.
1a1ff0f to
6ca4986
Compare
We must use its result asynchronously.
rpassis
left a comment
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.
Tested this with SQL_ENABLE_SNAPSHOT turned on and off.
In both scenarios the changes here drastically reduced the thread blocking we were experiencing with ValueObservation and SQLCipher when opening a database connection.
Turning the persistentReadOnlyConnections flag to true didn't seem to have a noticeable effect however it seems like it would be an effective solution to minimise opening of database connections for cases like ours where this operation can be process intensive.
I've left one minor comment on the documentation where you seem to be using a reentrant method but the documentation says otherwise.
Thanks for working on this fix so quickly, our team truly appreciates your efforts 👊👍💙
Co-authored-by: Rogerio de Paula Assis <[email protected]>
|
Thank you for being so supportive, @rpassis and your team!! Without you, nothing would have happened 😃🎉 Now, on to v6.10.0! |
|
v6.10.0 has shipped |
Since #1350 we no longer need to expose it and check for failures at runtime.
Since #1350 we no longer need to expose it and check for failures at runtime.
This PR completes #1248, and aims at solving #1256:
Enhances performances of
ValueObservationonDatabasePool, because it no longer opens a new database connection when it starts.Allows applications to keep read-only connections open.
Configurationhas gained apersistentReadOnlyConnectionsproperty. The default is false:DatabasePoolmanages read-only connections in order to spare memory. When true,DatabasePoolkeeps read-only connections open.When profiling your application reveals that a lot of time is spent opening a new SQLite connection at the wrong place:
persistentReadOnlyConnectionsto true.try dbPool.read { _ in }, for example, creates a read-only connection if no one is available yet. There's no built-in api for creating multiple read-only connections (open a feature request if you need this).invalidateReadOnlyConnections(), because this method closes read-only connections regardless of thepersistentReadOnlyConnectionsconfiguration.Since #1248, ValueObservation on DatabasePool creates a new database connection when it starts, on the thread that starts the observation. #1256 has revealed that opening a connection can take a long time, with unfortunate consequences for the main thread :-/
In this PR, ValueObservation on DatabasePool just uses one of the available read-only connections:
.immediateobservation still blocks the main thread until the initial value is fetched, but no time is spent opening a new connection if there is an available read-only connection. If there is no available read-only connection, the main thread is blocked until one is opened, or becomes available. That's wherepersistentReadOnlyConnectionsis useful: it helps finding an available read-only connection, as long as the app has already performed reads before the observation starts.persistentReadOnlyConnectionsis useful, because starting an observation asynchronously also needs an available read-only connection.When WAL snapshots are available, a long-running transaction is kept opened on the read-only connection, until we can acquire a write access, compare database versions, perform a fetch+notification if and only if the database was modified, and start observing the write transactions. The ability to keep a long-running transaction (that spans several database accesses) on an existing reader is a new (internal) feature of this PR.
When WAL snapshots are not available (Xcode 14.0 and macOS, and some SQLCipher/SQLite builds), a double notification of the initial value is always performed, because we can't prove that the database was not changed between the initial fetch and the write access (we need WAL snapshots for that).