django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #24018 -- SQLite backend supports the init_command option

Open linville opened this issue 2 years ago • 4 comments

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.

Ticket #24018 in Trac

linville avatar Sep 01 '21 21:09 linville

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 ⛵️!

github-actions[bot] avatar Sep 01 '21 21:09 github-actions[bot]

@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?

linville avatar Sep 03 '21 04:09 linville

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.

felixxm avatar Sep 03 '21 04:09 felixxm

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.

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 avatar Sep 08 '21 03:09 linville

@linville @felixxm Is there anything I can do to help move this forward?

bcail avatar Nov 23 '22 14:11 bcail

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.

bcail avatar Feb 07 '24 21:02 bcail

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.

anze3db avatar Feb 08 '24 16:02 anze3db

@linville Thanks for updates :+1: I left small suggestions.

felixxm avatar Feb 14 '24 13:02 felixxm

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.

gcollazo avatar Feb 15 '24 16:02 gcollazo

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.

felixxm avatar Feb 15 '24 16:02 felixxm

@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 avatar Feb 15 '24 17:02 linville

@linville Thanks :+1: Welcome aboard :boat:

felixxm avatar Feb 16 '24 05:02 felixxm