[Django] #33471: sqlite no-op migrations
Django
-----------------------------------------+------------------------
Reporter: David Szotten | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
while writing a test case for #33470 i found that for sqlite, even a
seemingly db-transparent change like adding `choices` still generates sql
(new table + insert + drop + rename) even though this shouldn't be needed.
on e.g. postgres the same migration generates no sql
--
Ticket URL: <https://code.djangoproject.com/ticket/33471>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+------------------------------------
Reporter: David Szotten | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------+------------------------------------
Changes (by Mariusz Felisiak):
* easy: 0 => 1
* stage: Unreviewed => Accepted
Comment:
It was missed in #25253 (see 9159d173c3822312c653db7ff5b9a94b14af1dca).
Adding `choices` to the `non_database_attrs` should fix it:
{{{#!diff
diff --git a/django/db/backends/base/schema.py
b/django/db/backends/base/schema.py
index 4cd4567cbc..822da656d3 100644
--- a/django/db/backends/base/schema.py
+++ b/django/db/backends/base/schema.py
@@ -1130,6 +1130,7 @@ class BaseDatabaseSchemaEditor:
# - adding only a db_column and the column name is not changed
non_database_attrs = [
'blank',
+ 'choices',
'db_column',
'editable',
'error_messages',
}}}
Would you like to prepare a patch? (a regression test is required).
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:1>
Django
Reporter: David Szotten | Owner: nobody
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by Mariusz Felisiak):
* type: Bug => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:2>
Django
--------------------------------------+------------------------------------
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by David Szotten):
oh i didn't realise that this was done globally by attr name. (though how
come pg does the right thing?)
could marking `choices` as a `non_database_attrs` adversely impact third
party fields that eg use a pg enum type to model `choices` values?
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:3>
Django
Reporter: David Szotten | Owner: Douglas
Type: | Mumme
Cleanup/optimization | Status: assigned
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Douglas Mumme):
* owner: nobody => Douglas Mumme
* status: new => assigned
Comment:
Hey there, I'm new to this and trying to get into contributing to Django,
but I would like to pick this ticket up and prepare a patch for it if no
one is working on it right now. Please let me know if there is a problem
with this, thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Douglas
Type: | Mumme
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Douglas Mumme):
Just made a pull request with the patch to fix this
https://github.com/django/django/pull/15404
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Douglas
Type: | Mumme
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Douglas Mumme):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Douglas
Type: | Mumme
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Adam Johnson (added)
* needs_better_patch: 0 => 1
Comment:
Replying to [comment:3 David Szotten]:
> could marking `choices` as a `non_database_attrs` adversely impact third
party fields that eg use a pg enum type to model `choices` values?
True, this would cause a regression for-party enum fields, e.g.
[https://github.com/adamchainz/django-mysql django-mysql's EnumField] a
similar field may exist for PostgreSQL or Oracle. It seems we can safely
overwrite `_field_should_be_altered()` only on SQLite.
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Douglas
Type: | Mumme
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam Johnson):
Indeed it would break Django-MySQL’s `EnumField` - [https://django-
mysql.readthedocs.io/en/latest/model_fields/enum_field.html docs] /
[https://github.com/adamchainz/django-
mysql/blob/main/src/django_mysql/models/fields/enum.py source].
If the "ignorable attrs" were extended on a per-field-class basis, that
could work. It would be a bigger patch but it could allow many custom
field classes to avoid unnecessary database changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Douglas
Type: | Mumme
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Douglas
Type: | Mumme
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by David Szotten):
do you know off the top of your head what mechanism makes it a no-op on pg
already?
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Douglas
Type: | Mumme
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:10 David Szotten]:
> do you know off the top of your head what mechanism makes it a no-op on
pg already?
There is nothing specific to PostgreSQL here, it's also a noop on MySQL
and Oracle. It's caused by remaking tables on SQLite that is necessary in
most of cases (see related ticket #32502). Overwriting
`_field_should_be_altered()` in the SQLite backend should do the trick.
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Douglas
Type: | Mumme
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* cc: Sarah Boyce (added)
* has_patch: 0 => 1
Comment:
I have added a PR which hopefully addresses the above comments:
https://github.com/django/django/pull/15561
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Boyce
Cleanup/optimization | Status: assigned
Component: Migrations | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: Douglas Mumme => Sarah Boyce
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Sarah
Type: | Boyce
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: David Szotten | Owner: Sarah
Type: | Boyce
Component: Migrations | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"65effbdb101714ac98b3f143eaccadd8e4f08361" 65effbdb]:
{{{
#!CommitTicketReference repository=""
revision="65effbdb101714ac98b3f143eaccadd8e4f08361"
Fixed #33471 -- Made AlterField operation a noop when changing "choices".
This also allows customizing attributes of fields that don't affect
a column definition.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33471#comment:15>