django
django copied to clipboard
Fixed #24018 -- SQLite backend supports the init_command option
Adds support to the SQLite backend for the init_command
option so user can set PRAGMA options when setting up the connection.
SQLite doesn't natively support the init_command
keyword (such as MySQL does), the command is just run right after the connection is established.
Hello @linville! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
@linville Thanks for the patch 👍 I can see one issue with this approach, we don't inform users when sth goes wrong.
Thanks for reviewing! Could you expand on "when sth goes wrong", not sure I'm following? Would this be if the user uses invalid pragma/syntax in the init_command
?
Thanks for reviewing! Could you expand on "when sth goes wrong", not sure I'm following? Would this be if the user uses invalid pragma/syntax in the
init_command
?
As in the ticket description, PRAGMA journal_mode=wal;
return wal
on success and delete
/memory
when it fails. We don't validate the output so a user is not aware that journal_mode
is unchanged. Maybe that's fine.
See also Carlton's comment.
As in the ticket description,
PRAGMA journal_mode=wal;
returnwal
on success anddelete
/memory
when it fails. We don't validate the output so a user is not aware thatjournal_mode
is unchanged. Maybe that's fine.
Looking at the MySQL backend behavior for init_command
, if something goes wrong the django.db.utils.ProgrammingError
is raised with the SQL statement which would be useful to alert the user to errors. Having the sqlite backend implementation of init_command
behave similar to the MySQL one would certainly be reasonable (though I suppose the opposite could be argued since it isn't Django or even the MySQL python module doing the validation but rather MySQL itself).
Thinking about approaches for this: whether a value is returned from a pragma statement varies on the pragma, some will return a value (e.g.: journal_mode
) and some will not (e.g.: cache_size
). It looks like an implementation of this would involve running two cursor executes for each pragma being set by the init_command
: first execute the pragma setting statement and then then running another cursor execute to get the pragma's value and check that it matches what was assigned (this code would have to split multiple pragma set statements in the init_command and also extract the pragma names, but the syntax is simple so this shouldn't be too complex).
If we'd like to do validation, I'd be happy to take a shot at an implementation of the above approach if it seems reasonable.
@linville @felixxm Is there anything I can do to help move this forward?
I'm not sure we'd need to validate the result of the PRAGMA
command. We don't verify the PRAGMA
statements that django always runs. The user could also manually verify the value if desired.
I think validating every statement in the init_command
isn't practical because we don't control what the users actually do. The way that the current PR is implemented there is nothing stopping users from writing CREATE TABLE/INSERT/UPDATE/... statements in the init_command
.
If we do want validation, then I think a better approach would be to expose the pragma settings that we want to support and validate through DATABASE OPTIONS e.g. DATABASE["default"]["OPTIONS"]["journal_mode"]
.
To me personally either approach works, I'd just really like to have this feature land in 5.1! Happy to help out in any way I can to get this done.
@linville Thanks for updates :+1: I left small suggestions.
Great work thanks!
Would it be possible/make sense to set or recommend (in docs) a "sensible default configuration" for using SQLite in the context of a web app?
Ruby on Rails being very opinionated, decided to set some defaults. To me this makes a lot sense since this is mostly the configuration I'm using in my web projects.
The only thing missing is setting the busy_timeout
but I think they have implemented a connection retry and backoff mechanism to deal with that at the framework layer instead of at the SQLite layer.
DEFAULT_PRAGMAS = {
"foreign_keys" => true,
"journal_mode" => :wal,
"synchronous" => :normal,
"mmap_size" => 134217728, # 128 megabytes
"journal_size_limit" => 67108864, # 64 megabytes
"cache_size" => 2000
}
Here's a link to the RoR settings for SQLite and here's a great blog post that helps understand the configuration and performs some benchmarks.
Would it be possible/make sense to set or recommend (in docs) a "sensible default configuration" for using SQLite in the context of a web app?
I don't think so. If we decided we needed more options we would set them automatically, as we already do:
https://github.com/django/django/blob/c991602ce5798385261381025c06698d7fd30dc5/django/db/backends/sqlite3/base.py#L200-L203
You can start a discussion on the Django Forum if you strongly believe that Django should set more options by default.
@linville Thanks for updates 👍 I left small suggestions.
Thank you for the review! I believe (hope!) I've got the requested changes incorporated now.
@linville Thanks :+1: Welcome aboard :boat: