Skip to content
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

SQLite: Ability to change journal mode #3399

Closed
pixelmatrix opened this issue Jun 26, 2024 · 11 comments · Fixed by apollographql/apollo-ios-dev#443
Closed

SQLite: Ability to change journal mode #3399

pixelmatrix opened this issue Jun 26, 2024 · 11 comments · Fixed by apollographql/apollo-ios-dev#443
Assignees
Labels
feature New addition or enhancement to existing solutions

Comments

@pixelmatrix
Copy link

pixelmatrix commented Jun 26, 2024

Use case

I've been investigating a regular background crash in our app. After some research, it appears to be due to keeping the SQLite database file located in the App Group's container. It looks like other people have fixed these crashes by using the database in WAL mode. This is possibly by executing a PRAGMA statement:

try db.execute("PRAGMA journal_mode = WAL;")

Describe the solution you'd like

Would you be open to exposing this as an option in some form?

A few API ideas:

  1. setJournalMode(_ mode: JournalMode) throws on SQLiteDatabase protocol
  2. execute(_ sql: String) throws on SQLiteDatabase, which would allow direct access to execute any PRAGMA command
  3. Add useWAL as an optional init parameter of SQLiteNormalizedCache

Alternatively, I could implement my own SQLiteDatabase, but that is currently not possible because DatabaseRow does not have a public initializer.

@pixelmatrix pixelmatrix added the feature New addition or enhancement to existing solutions label Jun 26, 2024
@calvincestari
Copy link
Member

Hi @pixelmatrix,

After some research, it appears to be due to keeping the SQLite database file located in the App Group's container. It looks like other people have fixed these crashes by using the database in WAL mode.

Can you share more evidence that points to the change in journaling mode being the solution to the crash. We're not against making this, or other pragmas, available but something conclusive that demonstrates this change resolves the issue will be helpful. Apollo iOS can be forked so making the change on your own fork while proving out the theory should be possible.

@pixelmatrix
Copy link
Author

@calvincestari yes, definitely. I've forked the repo and I'm running it in WAL mode in a recent TestFlight release.

So far the crashes have stopped, but it hasn't been long. I will continue to monitor for a week or so to see if we get any further crashes. Would that be enough evidence? I can also share some of the trace which points to the app being killed while SQLite is active.

@calvincestari
Copy link
Member

Yes, that sounds good. Monitor it, gather what you need to and then let's evaluate once it's had some time to prove itself.

@pixelmatrix
Copy link
Author

I wanted to share an update on this after living with the patch for some time. The crashes have decreased quite a bit, but haven't been entirely eradicated. After further research, it seems that even in WAL mode, we need to be careful not to leave the app in the middle of a write transaction when the app is suspended to prevent being killed by the system.

It looks like using the database in WAL journal mode is going to be necessary to prevent these crashes. GRDB (a separate SQLite implementation) has a guide on how to successfully share a SQLite db between processes here, which mentions needing to use WAL: https://swiftpackageindex.com/groue/grdb.swift/v6.27.0/documentation/grdb/databasesharing.

As far as addressing the transactions in the background, I'm still thinking about how to approach it. It seems like we need to wrap transactions in a background task, and cancel them if the task is expiring. Additionally, we need to prevent new transactions while the app is entering the background, ideally enqueuing them to run once the app is resumed.

I think a lot of this stems from using Subscriptions which can trigger a lot of cache writes. If a subscription comes through just as the app is leaving the foreground, it can leave a transaction open. It would be nice to be able to pause a subscription when the app is entering the background, or at least defer the cache write.

In summary, I still think supporting this mode is necessary for my use case, but there may be more work needed to properly support storing the SQLite db file in the App Group Container for sharing with other extensions.

@calvincestari
Copy link
Member

I'll look into exposing the ability to set the journal mode.

As for pausing subscriptions, you're either subscribed and receiving events or not. I've never seen any server implementation that would keep the subscription alive but not deliver any events. I'm curious about your subscriptions behaviour when entering the background though. Surely the websocket connections are killed and you have to restart the subscription when the app comes to the foreground again?

@pixelmatrix
Copy link
Author

Yeah, the websocket connection gets disconnected while the app is in the background. The crash logs show that SQLite transactions are being left open. There are other ways for this to happen, but my theory is that a subscription update comes through right as the app is transitioning into the background, which leaves a SQLite transaction open. There are some noisy subscriptions that could be firing nearly constantly for some users.

If background time is requested to complete the SQLite transaction, the socket may continue to live longer, so we may need to also manually disconnect the socket early to avoid further transactions. I think this is doable in the WebSocketTransport, but I haven't explored it quite yet.

@calvincestari
Copy link
Member

@pixelmatrix - I've got a draft PR up for this at apollographql/apollo-ios-dev#443. Please take a look and let me know if it's on the right track to satisfy what you're needing it to do. Thanks.

@pixelmatrix
Copy link
Author

That looks great! Thanks for looking into this

Copy link
Contributor

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo iOS usage and allow us to serve you better.

@calvincestari
Copy link
Member

The PR for this has been merged and the feature will go out in the next release, which we're planning to do this week still.

@pixelmatrix
Copy link
Author

Fantastic! Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants