Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Use SQLite in-memory temporary table store #6319

Closed
wants to merge 3 commits into from

Conversation

friedbunny
Copy link
Contributor

@friedbunny friedbunny commented Sep 13, 2016

Fixes #6193 — a crash on Android with terminating with uncaught exception of type mapbox::sqlite::Exception: unable to open database file. That crash was caused by Android’s SQLite trying to write to ./someTempFile, which it did not have permission to do. This was brought about by the change to WAL journaling in #5796.

This pull request sets PRAGMA temp_store = MEMORY, where previously it was the default (FILE). Using an in-memory temporary table store avoids the permission issue and fixes the crash. This should not affect the two WAL cache files.

Some background:

The temp_store values specifies the type of database back-end to use for temporary files. The choices are DEFAULT (0), FILE (1), and MEMORY (2). The use of a memory database for temporary tables can produce signifigant savings. DEFAULT specifies the compiled-in default, which is FILE unless the source has been modified.

This pragma will have no effect if the sqlite library has support for in-memory databases turned off using symbol SQLITE_OMIT_INMEMORYDB.

Why not change where temporary files are written?

Simply changing PRAGMA temp_store_directory isn’t a good option: it’s deprecated and there are no guarantees we could set it to a good temp directory. We would also have to limit this to Android, because other platforms do not have this problem (and use different temp dirs).

Benefits of temp_store = MEMORY

  • No longer crashes on Android — under standard conditions.
  • Supposedly faster.

Risks

  • Will increase memory usage by some unknown amount.
  • Android is the wild-west and an OEM could have compiled SQLite with SQLITE_OMIT_INMEMORYDB, which means SQLite could still try to write temporary files and subsequently crash.

To-do

  • Quantify memory usage.
  • Further research into when these temporary tables are created.
  • Research how common SQLITE_OMIT_INMEMORYDB is on different plaforms.
  • Get sign-off with testing for different platforms:
    • iOS
    • Android
    • Qt
    • Node
    • macOS

/cc @mapbox/gl

@friedbunny friedbunny added crash ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold Core The cross-platform C++ core, aka mbgl labels Sep 13, 2016
@friedbunny friedbunny added this to the android-v4.2.0 milestone Sep 13, 2016
@friedbunny friedbunny self-assigned this Sep 13, 2016
@mention-bot
Copy link

@friedbunny, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh to be potential reviewers.

@jfirebaugh
Copy link
Contributor

Is PRAGMA temp_store a persistent setting? My interpretation was it was transient, and perhaps even a library-wide (as opposed to per-database or per-connection) setting.

@jfirebaugh
Copy link
Contributor

Further research into when these temporary tables are created.

https://www.sqlite.org/tempfiles.html is quite informative.

We're likely hitting "2.5. Statement Journal Files" and possibly "2.7. Materializations Of Views And Subqueries" or "2.8. Transient Indices".

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Sep 13, 2016

Theory:

  • We're hitting "2.5. Statement Journal Files" exclusively.

  • The following statement did apply to our use before, but doesn't now that we're using WAL (there is no rollback journal):

    If the UPDATE or INSERT is not contained within BEGIN...COMMIT and if there are no other active statements on the same database connection then no statement journal is created since the ordinary rollback journal can be used instead.

This would explain our symptoms. But I'm not sure why a statement journal would be triggered for the query logged in #6193 (comment), which is here. I'd expect the query planner to recognize that the WHERE clause names a UNIQUE constraint and thus the statement will never update multiple rows, and the documentation claims updating multiple rows is a prerequisite for using a statement journal.

@friedbunny friedbunny removed this from the android-v4.2.0 milestone Sep 13, 2016
@friedbunny
Copy link
Contributor Author

Dropping this PR in favor of reverting WAL in #6320.

@friedbunny friedbunny closed this Sep 13, 2016
@friedbunny friedbunny deleted the fb-sqlite-memory-store branch September 13, 2016 18:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl crash ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sqlite exception unable to open database file
3 participants