Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#29182 closed Bug (fixed)

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

Reported by: ezaquarii Owned by: nobody
Component: Migrations Version: 2.0
Severity: Release blocker Keywords: sqlite migration
Cc: Simon Charette, Florian Apolloner, Adam Goode, Muflone Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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

Attachments (3)

bug_repro.sh (990 bytes ) - added by ezaquarii 6 years ago.
Bug reproduction steps as shell script
db_schema.sql (8.5 KB ) - added by ezaquarii 6 years ago.
Database schema after running migrations - see references to temporary openvpn_serverold table
initial_patch.diff (6.4 KB ) - added by Florian Apolloner 5 years ago.

Download all attachments as: .zip

Change History (51)

by ezaquarii, 6 years ago

Attachment: bug_repro.sh added

Bug reproduction steps as shell script

by ezaquarii, 6 years ago

Attachment: db_schema.sql added

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

comment:1 by ezaquarii, 6 years ago

Description: modified (diff)

comment:2 by Tim Graham, 6 years ago

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!

in reply to:  2 comment:3 by ezaquarii, 6 years ago

Description: modified (diff)

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.

Last edited 6 years ago by ezaquarii (previous) (diff)

comment:4 by ezaquarii, 6 years ago

Description: modified (diff)

comment:5 by ezaquarii, 6 years ago

Description: modified (diff)

comment:6 by Simon Charette, 6 years ago

Cc: Simon Charette added
Triage Stage: UnreviewedAccepted

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.

Last edited 6 years ago by Simon Charette (previous) (diff)

comment:7 by Tim Graham, 6 years ago

Type: UncategorizedCleanup/optimization

comment:8 by Florian Apolloner, 5 years ago

Severity: NormalRelease blocker
Type: Cleanup/optimizationBug

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.

comment:9 by Simon Charette, 5 years ago

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

comment:10 by Florian Apolloner, 5 years ago

Cc: Florian Apolloner added

comment:11 by Florian Apolloner, 5 years ago

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.

by Florian Apolloner, 5 years ago

Attachment: initial_patch.diff added

comment:12 by Florian Apolloner, 5 years ago

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…

comment:13 by Simon Charette, 5 years ago

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.

comment:14 by Florian Apolloner, 5 years ago

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

comment:15 by Simon Charette, 5 years ago

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

comment:16 by Simon Charette, 5 years ago

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

comment:17 by Christoph Trassl, 5 years ago

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

comment:18 by Simon Charette, 5 years ago

Version: 2.02.1

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

comment:19 by Florian Apolloner, 5 years ago

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)

comment:20 by Adam Goode, 5 years ago

Cc: Adam Goode added

comment:21 by Simon Charette, 5 years ago

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

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

comment:22 by Chris Lamb, 5 years ago

  • 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)

comment:23 by Simon Charette, 5 years ago

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.

in reply to:  22 comment:24 by Florian Apolloner, 5 years ago

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

comment:25 by Simon Charette, 5 years ago

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.

in reply to:  25 comment:26 by Florian Apolloner, 5 years ago

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.

comment:27 by Chris Lamb, 5 years ago

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

comment:28 by Simon Charette, 5 years ago

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

comment:29 by Florian Apolloner, 5 years ago

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

comment:30 by Simon Charette, 5 years ago

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.

comment:31 by Muflone, 5 years ago

Cc: Muflone added

comment:33 by Simon Charette, 5 years ago

Version: 2.12.0

comment:35 by Arthur Vuillard, 5 years ago

The patch from https://github.com/django/django/pull/10733 worked for me on django 2.0 too

Last edited 5 years ago by Arthur Vuillard (previous) (diff)

comment:36 by Tim Graham <timograham@…>, 5 years ago

Resolution: fixed
Status: newclosed

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.

comment:37 by Tim Graham <timograham@…>, 5 years ago

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.

comment:38 by Tim Graham <timograham@…>, 5 years ago

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.

comment:39 by Tim Graham, 5 years ago

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

comment:40 by Carlton Gibson <carlton.gibson@…>, 5 years ago

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.

comment:41 by Carlton Gibson <carlton.gibson@…>, 5 years ago

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.

comment:42 by Tim Graham <timograham@…>, 5 years ago

In 25a0781a:

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

comment:43 by johnthagen, 5 years ago

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

comment:44 by Florian Apolloner, 5 years ago

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

comment:45 by johnthagen, 5 years ago

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!

comment:46 by Benno Fünfstück, 4 years ago

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

comment:47 by Benno Fünfstück, 4 years ago

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

comment:48 by Tony S Yu, 4 years ago

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):

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.

Last edited 4 years ago by Tony S Yu (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top