search

elixir-sqlite / exqlite

An SQLite3 driver for Elixir
https://hexdocs.pm/exqlite
MIT License
217 stars 48 forks source link

Adds .maybe_set_pragma/3 #139

Closed warmwaffles closed 3 years ago

kevinlang commented 3 years ago

I don't think all PRAGMAs are persistent, and some are expected to be required during setup at each connection:

I think we only need to do this for PRAGMAs that modify the database header. By cross-referencing the fileformat docs with the pragma docs, I think the only really relevant one is the journal_mode one, as even the cache_size one, for example, is not persistent (only deprecated default_cache_size actually modifies the corresponding header value).

Can we change this to just do this for the journal_mode pragma, for now?

warmwaffles commented 3 years ago

Yeah I'm okay with that. I'll make the change soon.

On Mon, Apr 5, 2021, 9:46 AM Kevin Lang @.***> wrote:

I don't think all PRAGMAs are persistent, and some are expected to be required during setup at each connection:

- http://sqlite.1065341.n5.nabble.com/Which-pragmas-are-persistent-td95287.html

https://stackoverflow.com/questions/55158295/are-sqlite-pragma-settings-permanent-or-do-they-disapear-after-db-close

I think we only need to do this for PRAGMAs that modify the database header. By cross-referencing the fileformat docs with the pragma docs https://www.sqlite.org/pragma.html, I think the only really relevant one is the journal_mode one, as even the cache_size one, for example, is not persistent (only deprecated default_cache_size actually modifies the corresponding header value).

Can we change this to just do this for the journal_mode pragma, for now?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/elixir-sqlite/exqlite/pull/139#issuecomment-813432045, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKAHILEFPCGFHIXKUVI2DTHHEKZANCNFSM42M2KXOQ .

warmwaffles commented 3 years ago

@kevinlang I reverted the other maybe_set_pragmas and only apply it for journal_mode and cache_size. This is probably going to help with #138 some.

I wish the db connection pool on the ecto side would have the ability to specify how many of the pool can be writers with the number of read only. Probably difficult to implement.

kevinlang commented 3 years ago

Looks good, but I don't think we need it for cache_size - it is set per-connection.

See https://www.sqlite.org/pragma.html, which mentions only default_cache_size (deprecated) is persisted/shared across all connections.

kevinlang commented 3 years ago

I think we may still hit the "database is locked" issue with these changes the first time the database is ever accessed, since we have no mutex and thus could have a race condition. But it should fix it for all subsequent runs.

We can maybe fix the first-run scenario by having the ecto adapter storage_up which calls Exqlite.Sqlite3.open to perhaps instead call Exqlite.Connection.connect, to do this setup at that time which I think only uses one connection. Will note for later

warmwaffles commented 3 years ago

We can maybe fix the first-run scenario by having the ecto adapter storage_up which calls Exqlite.Sqlite3.open to perhaps instead call Exqlite.Connection.connect, to do this setup at that time which I think only uses one connection. Will note for later

That's actually a pretty good idea