Check deferred foreign key constraints before dropping them

[Joey Wilhelm]

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:

        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()
>>> ''

[Joey Wilhelm]

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?

[Shai Berger]

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.

[Joey Wilhelm]

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?

[Tim Graham]

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.

[Shai Berger]

Replying to 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.

[Simon Charette]

I just wanted to mention that wrapping your migration operations with SET CONSTRAINTS ALL IMMEDIATE solves the pending trigger events message.

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?

[Simon Charette]

Here's a ​PR solving the pending trigger events issue by checking deferred foreign key constraints before deleting them.

[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).

[Simon Charette]

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 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.

[Simon Charette <charette.s@…>]

In cd7efa20:

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.