SQLite 3.26 breaks database migration ForeignKey constraint, leaving <table_name>__old in db schema

[ezaquarii]

SQLite table alteration uses rename and copy method. When running a database migration from Django command (via call_command('migrate')), database is left with references to <table_name>__old table that has been dropped after migration.

This happens only when running SQLite DB migrations from management command under transaction.
Running migrations via ./manage.py migrate does not cause any issues.

This is the demonstration:
​https://github.com/ezaquarii/django-sqlite-migration-bug

Steps to reproduce the bug

1. run ./bootstrap.sh
2. from import app.models import *
3. Bravo.objects.create()
4. SQL cannot be executed due to missing table main.app_alpha__old

• app/management/commands/bootstrap.py:9: run the migration
• alpha table is created
• bravo table is created; it uses foreign key to alpha
• another alpha migration is run, adding dummy field
• alpha table is renamed to alpha__old
• bravo foreign key is re-linked to alpha__old
• new table alpha is created and data from alpha__old is copied with applied alterations
• alpha__old is dropped
• bravo still references alpha__old - ForeignKey constraint is not updated and DB is left in unusable state

CREATE TABLE "app_bravo" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "dummy" varchar(10) NOT NULL, "alpha_id" integer NOT NULL REFERENCES "app_alpha__old" ("id") DEFERRABLE INITIALLY DEFERRED);

Tested with Django versions: 2.0.1 and 2.0.2

[ezaquarii]

Bug reproduction steps as shell script

[ezaquarii]

Database schema after running migrations - see references to temporary openvpn_serverold table

[Tim Graham]

Could you provide a more minimal project that reproduces the issue? In particular, the steps to reproduce shouldn't require installing a bunch of NodeJS packages. Thanks!

[ezaquarii]

Please check this repository:

​https://github.com/ezaquarii/django-sqlite-migration-bug

Please check out bootstrap.py

        with transaction.atomic():
            call_command('migrate')

I think this is the issue. It works ok without transaction.

[Simon Charette]

Hey ezaquarii, thank you for your great report.

You can get a bit of context about why all the SQLite constraint workarounds are required in #28849 but in this case I suspect odd things happen because, as you might know, savepoints are used in nested atomic() blocks.

The migration executor was not designed to be used within a transaction as it does some pretty intense transaction management itself so I'm accepting on the basis that we should do a better job at raising an error in this case. The fact that this doesn't happen when ./manage.py migrate is called directly is a good indicator that other weird things might be happening under the hood because this case is completely untested by the suite right now. For example, do the backends that we assume have transactional DDL support all behave the same way with regards to DDL in savepoints?

I guess you were trying to come up with some kind of deploying script that either fails or succeeds to apply multiple migrations?

I think the best place to raise the error from would be in sqlite3.DatebaseSchemaEditor.__enter__ when disable_constraint_checking returns False to signify the operation didn't succeed as database alteration methods the returned instance exposes all assume foreign key constraints are disabled. FWIW the reason why disable_constraint_checking returns false within a transaction is that PRAGMA foreign_keys = ON/OFF has absolutely no effect in a transaction which is one of the reason why the workarounds in #28849 had to be implemented.

[Florian Apolloner]

Changing this to a full blown bug and release blocker since I am able to reproduce this with ./manage.py migrate and very simple app.

Relevant versions:
Sqlite: 3.26.0 2018-12-01 12:34:55 bf8c1b2b7a5960c282e543b9c293686dccff272512d08865f4600fb58238alt1
python sqlite3 version: 2.6.0

Same test models:

from django.db import models

class File(models.Model):
    content = models.ForeignKey("FileContent", on_delete=models.PROTECT)

class FileContent(models.Model):
    pass

class PlayBook(models.Model):
    file = models.ForeignKey("File", on_delete=models.PROTECT)

This generates the following initial migration (on latest master):

# Generated by Django 2.2.dev20181204152138 on 2018-12-04 16:49

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

    initial = True

    dependencies = [
    ]

    operations = [
        migrations.CreateModel(
            name='File',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
            ],
        ),
        migrations.CreateModel(
            name='FileContent',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
            ],
        ),
        migrations.CreateModel(
            name='PlayBook',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('file', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='testing.File')),
            ],
        ),
        migrations.AddField(
            model_name='file',
            name='content',
            field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='testing.FileContent'),
        ),
    ]

Running this migration results in the following schema in SQLite:

CREATE TABLE IF NOT EXISTS "django_migrations"(
  "id" integer NOT NULL PRIMARY KEY AUTOINCREMENT,
  "app" varchar(255) NOT NULL,
  "name" varchar(255) NOT NULL,
  "applied" datetime NOT NULL
);
CREATE TABLE IF NOT EXISTS "testing_filecontent"(
  "id" integer NOT NULL PRIMARY KEY AUTOINCREMENT
);
CREATE TABLE IF NOT EXISTS "testing_playbook"(
  "id" integer NOT NULL PRIMARY KEY AUTOINCREMENT,
  "file_id" integer NOT NULL REFERENCES "testing_file__old"("id") DEFERRABLE INITIALLY DEFERRED
);
CREATE TABLE IF NOT EXISTS "testing_file"(
  "id" integer NOT NULL PRIMARY KEY AUTOINCREMENT,
  "content_id" integer NOT NULL REFERENCES "testing_filecontent"("id") DEFERRABLE INITIALLY DEFERRED
);
CREATE INDEX "testing_playbook_file_id_debe0476" ON "testing_playbook"(
  "file_id"
);
CREATE INDEX "testing_file_content_id_4682b86d" ON "testing_file"(
  "content_id"
);

From the looks of it, we will need to do some similar to ​https://github.com/django/django/blob/196b420fcb0cbdd82970e2b9aea80251bde82056/django/db/backends/sqlite3/schema.py#L108-L121 whenever we rename a table which has foreignkeys pointing to it.

[Simon Charette]

Thanks for the extra details Florian. I'll try to have a look at it this week.

[Florian Apolloner]

Fwiw I think we will have to disable can_rollback_ddl on sqlite to get a sane behavior, otherwise pretty much every migration out there will break.

[Florian Apolloner]

I have added an initial patch which fixes my issue -- it should serve only as starting point for some ideas and also has a few questions left…

[Simon Charette]

Florian, if you still have a few minutes to dedicate to this issue I'd appreciate if you could add a ​schema test based on your investigation reproducing the issue. It's surprising that this wasn't caught by an existing foreign key addition test.

[Florian Apolloner]

I'll try to do so tomorrow, I've never written a schema test :)

[Simon Charette]

I did a bit of investigation on my side and there must be something else at play here as I cannot reproduce against stable/2.1.x and master with SQLite version 3.22.0 2018-01-22 18:45:57 and sqlite3.version 2.6.0 on Python 3.6.6.

$ sqlite3 project/db.sqlite3 
SQLite version 3.22.0 2018-01-22 18:45:57
Enter ".help" for usage hints.
sqlite> .schema
CREATE TABLE IF NOT EXISTS "django_migrations" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "app" varchar(255) NOT NULL, "name" varchar(255) NOT NULL, "applied" datetime NOT NULL);
CREATE TABLE sqlite_sequence(name,seq);
CREATE TABLE IF NOT EXISTS "ticket_29182_filecontent" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT);
CREATE TABLE IF NOT EXISTS "ticket_29182_playbook" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "file_id" integer NOT NULL REFERENCES "ticket_29182_file" ("id") DEFERRABLE INITIALLY DEFERRED);
CREATE TABLE IF NOT EXISTS "ticket_29182_file" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "content_id" integer NOT NULL REFERENCES "ticket_29182_filecontent" ("id") DEFERRABLE INITIALLY DEFERRED);
CREATE INDEX "ticket_29182_playbook_file_id_f3bc0e50" ON "ticket_29182_playbook" ("file_id");
CREATE INDEX "ticket_29182_file_content_id_7815674c" ON "ticket_29182_file" ("content_id");

I also couldn't get a regression test to fail either. Florian could you confirm one of these added tests fail for you on your local setup: ​https://github.com/django/django/compare/master...charettes:ticket-29182

[Simon Charette]

Cannot reproduce with SQLite 3.23.1 and 3.24.0 2018-06-04 14:10:15 either, I'll try to get my hands on 3.26+ tomorrow. Could you provide the output of

import sqlite3
sqlite3.connect('file::memory:').cursor().execute('PRAGMA compile_options').fetchall()

[Christoph Trassl]

It's broken with SQLite 3.26. SQLite 3.25.3 is fine, regardless of Django version. Perhaps it is related to changes to the ALTER TABLE statement in SQLite 3.26 (​https://sqlite.org/pragma.html#pragma_legacy_alter_table).

[Simon Charette]

Thanks for confirming Christoph, that explains why CI didn't break. In this case the safest option is probably to turn this prama on for Django 2.1 and adjust the schema altering logic on 2.2+.

[Florian Apolloner]

Simon, the following test is the best I could come up with -- not sure if there is a better way aside from querying sqlite_master, but seems to be okay for the purpose of the test.

    @isolate_apps('schema')
    @unittest.skipUnless(connection.vendor == 'sqlite', 'SQLite naively remakes the table on field alteration.')
    def test_table_remake_with_fks_pointing_to_it(self):
        class File(Model):
            content = ForeignKey("FileContent", on_delete=PROTECT)

            class Meta:
                app_label = 'schema'

        class FileContent(Model):
            class Meta:
                app_label = 'schema'

        class PlayBook(Model):
            file = ForeignKey("File", on_delete=PROTECT)

            class Meta:
                app_label = 'schema'

        new_field = ForeignKey(FileContent, on_delete=PROTECT)
        new_field.set_attributes_from_name("filecontent")

        with connection.schema_editor(atomic=True) as editor:
            editor.create_model(File)
            editor.create_model(FileContent)
            editor.create_model(PlayBook)
            editor.add_field(File, new_field)

        with connection.cursor() as cursor:
            cursor.execute("SELECT sql FROM sqlite_master WHERE type='table' and tbl_name='schema_playbook'")
            result = cursor.fetchone()[0]

        self.assertNotIn("__old", result)

[Simon Charette]

Changed ticket name to give it more exposure as there have been a few duplicates so far (e.g. #30016).

[Chris Lamb]

• Apologies for the dupe. I filed #30016after not reading the most-recent entries here more carefully...

• I can reproduce with sqlite3 3.26.0 (but not with 3.25.3)

However, the patch in https://code.djangoproject.com/ticket/29182#comment:12 does not work for me:

test_testcase_ordering (test_runner.test_discover_runner.DiscoverRunnerTest) ... ok
test_transaction_support (test_runner.tests.Sqlite3InMemoryTestDbs)
Ticket #16329: sqlite3 in-memory test databases ... ERROR

======================================================================
ERROR: test_transaction_support (test_runner.tests.Sqlite3InMemoryTestDbs)
Ticket #16329: sqlite3 in-memory test databases
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/lamby/git/debian/python-team/modules/python-django/tests/test_runner/tests.py", line 226, in test_transaction_support
    DiscoverRunner(verbosity=0).setup_databases()
  File "/home/lamby/git/debian/python-team/modules/python-django/django/test/runner.py", line 551, in setup_databases
    self.parallel, **kwargs
  File "/home/lamby/git/debian/python-team/modules/python-django/django/test/utils.py", line 174, in setup_databases
    serialize=connection.settings_dict.get('TEST', {}).get('SERIALIZE', True),
  File "/home/lamby/git/debian/python-team/modules/python-django/django/db/backends/base/creation.py", line 68, in create_test_db
    run_syncdb=True,
  File "/home/lamby/git/debian/python-team/modules/python-django/django/core/management/__init__.py", line 148, in call_command
    return command.execute(*args, **defaults)
  File "/home/lamby/git/debian/python-team/modules/python-django/django/core/management/base.py", line 353, in execute
    output = self.handle(*args, **options)
  File "/home/lamby/git/debian/python-team/modules/python-django/django/core/management/base.py", line 83, in wrapped
    res = handle_func(*args, **kwargs)
  File "/home/lamby/git/debian/python-team/modules/python-django/django/core/management/commands/migrate.py", line 172, in handle
    self.sync_apps(connection, executor.loader.unmigrated_apps)
  File "/home/lamby/git/debian/python-team/modules/python-django/django/core/management/commands/migrate.py", line 306, in sync_apps
    editor.create_model(model)
  File "/home/lamby/git/debian/python-team/modules/python-django/django/db/backends/base/schema.py", line 312, in create_model
    self.execute(sql, params or None)
  File "/home/lamby/git/debian/python-team/modules/python-django/django/db/backends/base/schema.py", line 118, in execute
    "Executing DDL statements while in a transaction on databases "
django.db.transaction.TransactionManagementError: Executing DDL statements while in a transaction on databases that can't perform a rollback is prohibited.

----------------------------------------------------------------------
Ran 5020 tests in 138.176s

FAILED (errors=1, skipped=657, expected failures=3)

[Simon Charette]

Hello Chris,

I don't think the approach of the patch https://code.djangoproject.com/ticket/29182#comment:12 will work.

Could you give ​https://github.com/django/django/compare/master...charettes:ticket-29182-pragma a run against the test suite on SQLite 3.26.

[Florian Apolloner]

Replying to Chris Lamb:

However, the patch in https://code.djangoproject.com/ticket/29182#comment:12 does not work for me:

As said this is just a starting point and we might come around to make this actually the expected behavior. We cannot safely alter a SQLite database inside a transaction from the looks of it (If you look at the patch we rewrite internal tables and then need to issues a VACUUM -- not sure if we can do that safely in transactions).

[Simon Charette]

We cannot safely alter a SQLite database inside a transaction from the looks of it...

It has been working just fine on SQLite < 3.26 and the tests prove it.

The current issue with 3.26 is that it changes the assumptions related to foreign key repointing on table rename when constraint checks are disabled. On SQLite < 3.26 renaming a table like we do in table remake because of unsupported ALTER table doesn't automatically repoint the foreign key constraints (e.g from table_name to table_name__old) but it does on SQLite 3.26.

[Florian Apolloner]

Replying to Simon Charette:

We cannot safely alter a SQLite database inside a transaction from the looks of it...

It has been working just fine on SQLite < 3.26 and the tests prove it.

Yes, I was speaking with my 3.26 hat on (That is if we want to fix it without the PRAGMA for the rename). The existing code in master absolutely works fine and is stable for anything but the newest SQLite.

[Chris Lamb]

This is being tracked upstream in sqlite3 here: ​https://www.sqlite.org/src/info/f44bc7a8b3fac82a (hat-tip László Böszörményi in ​https://bugs.debian.org/915626#27).

[Simon Charette]

Florian or Chris,

Could you give a try at running the suite with the following patch and report if it helps

​https://github.com/django/django/compare/master...charettes:ticket-29182-pragma

[Florian Apolloner]

Hrmpf, I must have forgot to hit send before. Yes your pragma patch fixes it (with my testcase added).

[Simon Charette]

Awesome, thanks for confirming Florian. I'll turn it into a PR with a release note for the next 2.1 release later today.

I feel like we'll want to stick to this solution until we drop support for < 3.26 as the logic involved, as you've come to notice yourself, is already quite convoluted and adding some version branching in there would make it even worst. We've got a solution that allows us to keep atomic migrations enabled on SQLite so I suggest we stick to it.

[Simon Charette]

​https://github.com/django/django/pull/10733

[Chris Lamb]

​​https://github.com/django/django/pull/10733

Works for me.

[Arthur Vuillard]

The patch worked for me on django 2.0 too

[Tim Graham <timograham@…>]

In 53b17d4:

[2.0.x] Fixed #29182 -- Fixed schema table alteration on SQLite 3.26+.

SQLite 3.26 repoints foreign key constraints on table renames even when
foreign_keys pragma is off which breaks every operation that requires
a table rebuild to simulate unsupported ALTER TABLE statements.

The newly introduced legacy_alter_table pragma disables this behavior
and restores the previous schema editor assumptions.

Thanks Florian Apolloner, Christoph Trassl, Chris Lamb for the report and
troubleshooting assistance.

Backport of c8ffdbe514b55ff5c9a2b8cb8bbdf2d3978c188f from master.

[Tim Graham <timograham@…>]

In fc8c2284:

[2.1.x] Fixed #29182 -- Fixed schema table alteration on SQLite 3.26+.

SQLite 3.26 repoints foreign key constraints on table renames even when
foreign_keys pragma is off which breaks every operation that requires
a table rebuild to simulate unsupported ALTER TABLE statements.

The newly introduced legacy_alter_table pragma disables this behavior
and restores the previous schema editor assumptions.

Thanks Florian Apolloner, Christoph Trassl, Chris Lamb for the report and
troubleshooting assistance.

Backport of c8ffdbe514b55ff5c9a2b8cb8bbdf2d3978c188f from master.

[Tim Graham <timograham@…>]

In c8ffdbe:

Fixed #29182 -- Fixed schema table alteration on SQLite 3.26+.

SQLite 3.26 repoints foreign key constraints on table renames even when
foreign_keys pragma is off which breaks every operation that requires
a table rebuild to simulate unsupported ALTER TABLE statements.

The newly introduced legacy_alter_table pragma disables this behavior
and restores the previous schema editor assumptions.

Thanks Florian Apolloner, Christoph Trassl, Chris Lamb for the report and
troubleshooting assistance.

[Tim Graham]

The original issue reported in this ticket will be addressed in #30023. The ticket was repurposed incorrectly for a different issue.

[Carlton Gibson <carlton.gibson@…>]

In 7289874a:

Fixed #30033 -- Conformed to the recommended table alterations procedure on SQlite3.

Refs #29182.

The previous implementation was following a procedure explicitly documented
as incorrect and was the origin of the breakage experienced on SQLite 3.26
release that were addressed by c8ffdbe514b55ff5c9a2b8cb8bbdf2d3978c188f.

Thanks to Richard Hipp for pointing out the usage of the incorrect procedure.

[Carlton Gibson <carlton.gibson@…>]

In 894cb13:

Refs #29182 -- Stopped relying on legacy alter table semantic on SQLite 3.26+.

SQLite 3.26 changed the behavior of table and column renaming operations to
repoint foreign key references even if foreign key checks are disabled.

This makes the workarounds in place to simulate this behavior unnecessary on
SQLite 3.26+. Refs #30033.

[Tim Graham <timograham@…>]

In 25a0781a:

Refs #29182 -- Corrected SQLite's supports_atomic_references_rename feature flag.

[johnthagen]

FWIW, I hit this bug too in Python 3.7.1, sqlite 3.26.0, and Django 2.1.4.

In my case, if I had models with ForeignKeys that related to each other in reverse alphabetic order, I got errors on that looked like:

django.db.utils.OperationalError: no such table: main.polls_cat__old

In case it's helpful for another data point, I wrote a detailed report here: ​https://github.com/beda-software/drf-writable-nested/issues/64

[Florian Apolloner]

The fix is committed but not yet in any released version afaik; so you'd have to test against master…

[johnthagen]

I just tried it out my minimal example using pip install git+https://github.com/django/django.git

$ pip list
Package               Version              
--------------------- ---------------------
Django                2.2.dev20181230164339
...

And it seems to be fixed! I will double check with 2.1.5 as well after it's released Jan 1. Thanks!

[Benno Fünfstück]

This is still broken in Django 1.11.25, shouldn't it be backported to that version as well?

[Benno Fünfstück]

I created a PR to backport to 1.11: ​https://github.com/django/django/pull/11986

[Tony S Yu]

The backport for the fix to Django 1.11 was ​closed. I commented on that ticket, but for visibility, I'll comment here as well.

This issue makes it difficult to test libraries supporting both Django 1.11 and Django 2.2 on a single Centos server (without hacks to upgrade supported sqlite versions):

• Centos 7 supports sqlite3 3.7.17 (see ​EL-7 here)
• Centos 8 supports sqlite3 3.26.0 (see ​EL-8 here)

Our team runs internal libraries on Centos servers using tox to test against various versions of dependencies. The current version incompatibilities mean that Django 1.11 tests of a library must run on Centos 7 and tests for Django 2.2 must run on Centos 8.