#33379 closed New feature (fixed)
Add minimum database version checks
Reported by: | Tim Graham | Owned by: | Hasan Ramezani |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
7444f3252757ed4384623e5afd7dcfeef3e0c74e added a minimum version check for SQLite, but the other database backends don't have such a check. We sometimes get bug reports from people using an unsupported database version, so I think the checks would add value. Is a query to fetch the database version on startup an acceptable cost?
Perhaps there's a better way to run a query just once, but here's what I did in django-cockroachdb:
https://github.com/cockroachdb/django-cockroachdb/commit/27ebbefa515edf3ba68a5373dea48c4acdda60ab
We could make the check generic by adding DatabaseFeatures.minimum_database_version
as well as a standard method to get the database version as suggested in #18332.
Change History (16)
comment:1 by , 3 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 3 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
I've created a draft patch and implemented the check for the SQLite backend.
@Tim, please check it and let me know if the implementation and if you are ok I can continue with the other backends.
comment:5 by , 3 years ago
Patch needs improvement: | set |
---|
comment:7 by , 3 years ago
Patch needs improvement: | set |
---|
comment:8 by , 3 years ago
Patch needs improvement: | unset |
---|
comment:9 by , 3 years ago
Patch needs improvement: | set |
---|
As noted on the PR, I tested this with CockroachDB and it doesn't work because the system check framework runs after other initialization queries, some of which fail on older versions of CockroachDB. Perhaps moving the error to another method like DatabaseWrapper.init_connection_state() as I did for CockroachDB (implementation linked in ticket description) is more suitable.
comment:10 by , 3 years ago
Patch needs improvement: | unset |
---|
I've added a new commit to the existing patch and moved the database version check to init_connection_state
.
Also, I've changed the tests.
comment:11 by , 3 years ago
Patch needs improvement: | set |
---|
comment:12 by , 3 years ago
Patch needs improvement: | unset |
---|
comment:13 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Makes sense, thanks. IMO an extra query on startup is acceptable.