[Django] #25492: makemigrations can generate invalid migrations when removing null=True from a field
Django
a field
-----------------------------+--------------------
Reporter: tarkatronic | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------
Using Django 1.8.4 and PostgreSQL 9.4.4, I have run into a scenario where
a migrations was created which generated an error when I attempted to
apply it. The relevant operation from the migration is:
{{{#!python
migrations.AlterField(
model_name='charity',
name='name_legal',
field=models.CharField(max_length=255, default='',
verbose_name='legal name'),
preserve_default=False,
),
}}}
The actual change was to remove `null=True` and `blank=True` from the
field.
When running the migration, I get the following error:
{{{django.db.utils.OperationalError: cannot ALTER TABLE "charity" because
it has pending trigger events}}}
I can provide full traceback if necessary; I just didn't want to provide
excess on here.
From what I have found, this is because the field is being both altered
and updated inside the same transaction, as per this SO answer:
http://stackoverflow.com/a/12838113/1971587
Knowing this, I know that I need to create an intermediate migration in
order to set defaults on existing rows prior to the alter. However, I
don't believe that it should have been possible to create a migration
which is inherently broken.
When creating the migration, I don't believe that I should have been
presented with the first option here:
{{{
You are trying to change the nullable field 'name_legal' on charity to
non-nullable without a default; we can't do that (the database needs
something to populate existing rows).
Please select a fix:
1) Provide a one-off default now (will be set on all existing rows)
2) Ignore for now, and let me handle existing rows with NULL myself (e.g.
adding a RunPython or RunSQL operation in the new migration file before
the AlterField operation)
3) Quit, and let me add a default in models.py
Select an option: 1
Please enter the default value now, as valid Python
The datetime and django.utils.timezone modules are available, so you can
do e.g. timezone.now()
>>> ''
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25492>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
a field
Reporter: tarkatronic | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tarkatronic):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Actually, it looks like this is related to #24630 and #25453. Possibly a
duplicate of them. Just a slightly different view/approach on it. My bad,
I should have searched better before posting.
Regardless, given that this has come up multiple times, should it be made
more difficult to create these invalid migrations, rather than simply
documenting the fact that you can easily create them?
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:1>
Django
-----------------------------+--------------------------------------
Reporter: tarkatronic | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* version: 1.8 => master
* type: Bug => New feature
Comment:
The problem is that some migrations, even including both schema changes
and data changes, are valid; for an example, see comment:5:ticket:24630.
So, forbidding such migrations is a backwards-incompatible change.
That said, I still think it is a change worth making, at least to some
degree. I think that, for starters, a warning (using the Checks framework)
for mixed operations in a migration could be a good idea. To support it,
we'd need each operation to define its "type" -- schema or data operation,
or "possibly either" (e.g. `RunSQL`); the check would warn if any
migration mixes types, or if a "possibly either" operation is in the same
migration with any other operation. At least for the "possibly either"
operations, the type should be overridable. All these APIs need to be
public, to enable user-defined operations to play nice.
I've modified the ticket title and type in accordance with this
suggestion; if this seems reasonable, we should also change the
description. In any case, a behavior change about this would be a new
feature, and we'd need a reason to backport it even to 1.9, not to mention
1.8.
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:2>
Django
Reporter: tarkatronic | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* component: Migrations => Core (System checks)
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:3>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tarkatronic):
An alternative which was suggested, in regards to my specific case, was to
split the data and schema operations into separate transactions, or even
separate migrations. Would something like that be a possibility?
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:4>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Each migration is run in a separate transaction, so yes, splitting the
operations into separate files is what you should do. We don't want to run
multiple transactions per migration because we need each migration to be
atomic.
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:5>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* cc: shaib (added)
Comment:
Replying to [comment:4 tarkatronic]:
> An alternative which was suggested, in regards to my specific case, was
to split the data and schema operations into separate transactions, or
even separate migrations. Would something like that be a possibility?
If you're thinking about some automatic process to do the splitting into
migrations, I don't think that's a very good idea. Remember, to get the
problem in the first place, the migration needs to have been edited by the
user. This means that, in order to generate the separate migration files,
we'd need to parse and understand their code, and I suspect the fringe
cases where this would break (not as in "error out", but as in "generate
wrong code") are many and hard to detect.
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:6>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I just wanted to mention that wrapping your migration operations with `SET
CONSTRAINTS ALL IMMEDIATE` solves the `pending trigger events` message.
{{{#!python
operations = [
migrations.RunSQL('SET CONSTRAINTS ALL IMMEDIATE', 'SET CONSTRAINTS
ALL DEFERRED'),
migrations.AlterField(
model_name='charity',
name='name_legal',
field=models.CharField(max_length=255, default='',
verbose_name='legal name'),
preserve_default=False,
),
migrations.RunSQL('SET CONSTRAINTS ALL DEFERRED', 'SET CONSTRAINTS ALL
IMMEDIATE'),
]
}}}
Maybe this is something that could be done automatically by Django in
`AlterField` when it knows it has to perform both a data and structural
update?
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: Joey Wilhelm | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
Comment:
Here's a [https://github.com/django/django/pull/7709 PR] solving the
pending trigger events issue by checking deferred foreign key constraints
before deleting them.
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:8>
Django
Reporter: Joey Wilhelm | Owner: nobody
Type: New feature | Status: new
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Joey Wilhelm | Owner: nobody
Type: New feature | Status: new
Component: Core (System | Version: master
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Joey Wilhelm | Owner: nobody
Type: New feature | Status: new
Component: Core (System | Version: master
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger):
Haven't got time to review the patch, just wanted to note that mixing
schema-altering operations with data-modifying operations can have other
issues, mostly related to deferred SQL (statements that an operation
queues to be performed only at the end of the migration; FK creation does
that, and probably some others too).
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:11>
Django
Reporter: Joey Wilhelm | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* stage: Ready for checkin => Accepted
Comment:
I adjusted the patch to perform `SET CONSTRAINT %(name)s IMMEDIATE` before
dropping a constraint only on PostgreSQL as it's the only backend that
supports both deferrable foreign keys and transactional DDL which are the
two conditions required for this issue to arise.
I used feature detection with `skipUnlessDbFeature` to make sure the test
will be run on SQLite If we were to make foreign keys `DEFERRABLE
INITIALLY DEFERRED`
[https://code.djangoproject.com/ticket/14204#comment:19 like they are on
PostgreSQL and Oracle].
@shai
> Haven't got time to review the patch, just wanted to note that mixing
schema-altering operations with data-modifying operations can have other
issues, mostly related to deferred SQL
Don't worry, this patch isn't about encouraging both types of alteration
to be performed in the same migration which is another can of worm. It's
focusing on making sure we perform the pending checks on foreign key
constraints before dropping them.
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:12>
Django
Reporter: Joey Wilhelm | Owner: nobody
Type: New feature | Status: new
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Joey Wilhelm | Owner: nobody
Component: Core (System | Version: master
checks) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"cd7efa20338cb6f3ede4780e00590c0a6dd48ca2" cd7efa20]:
{{{
#!CommitTicketReference repository=""
revision="cd7efa20338cb6f3ede4780e00590c0a6dd48ca2"
Fixed #25492 -- Checked deferred foreign key constraints before dropping
them.
This allows running foreign key data and schema altering operations in the
same migration on PostgreSQL.
Thanks Tim for review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25492#comment:14>