makemigrations fails on special database connections

[Markus Gerards]

As direx in #27110 I have a bunch of database connections defined in settings.DATABASES which I'm using only with raw queries. One of them is a sphinxsearch engine. You can use a subset of MySQL commands to query the indexes, but not all MySQL commands are implemented there. So makemigrations fails when checking for the database schema:

django.db.utils.ProgrammingError: (1064, "sphinxql: syntax error, unexpected IDENT, expecting VARIABLES near 'FULL TABLES'")

I know, this will be some kind of edge-case but it would be helpful to skip any migration-related tasks for selected databases.

[Claude Paroz]

Could you please provide the full traceback of the error? Did you test the patch proposed for #27110, did it help in your use case?

[Markus Gerards]

Replying to claudep:

Could you please provide the full traceback of the error? Did you test the patch proposed for #27110, did it help in your use case?

This is the full traceback:

Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 367, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 359, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python2.7/site-packages/django/core/management/base.py", line 305, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python2.7/site-packages/django/core/management/base.py", line 356, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python2.7/site-packages/django/core/management/commands/makemigrations.py", line 100, in handle
    loader.check_consistent_history(connection)
  File "/usr/local/lib/python2.7/site-packages/django/db/migrations/loader.py", line 278, in check_consistent_history
    applied = recorder.applied_migrations()
  File "/usr/local/lib/python2.7/site-packages/django/db/migrations/recorder.py", line 65, in applied_migrations
    self.ensure_schema()
  File "/usr/local/lib/python2.7/site-packages/django/db/migrations/recorder.py", line 52, in ensure_schema
    if self.Migration._meta.db_table in self.connection.introspection.table_names(self.connection.cursor()):
  File "/usr/local/lib/python2.7/site-packages/django/db/backends/base/introspection.py", line 57, in table_names
    return get_names(cursor)
  File "/usr/local/lib/python2.7/site-packages/django/db/backends/base/introspection.py", line 52, in get_names
    return sorted(ti.name for ti in self.get_table_list(cursor)
  File "/usr/local/lib/python2.7/site-packages/django/db/backends/mysql/introspection.py", line 53, in get_table_list
    cursor.execute("SHOW FULL TABLES")
  File "/usr/local/lib/python2.7/site-packages/django/db/backends/utils.py", line 79, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "/usr/local/lib/python2.7/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "/usr/local/lib/python2.7/site-packages/django/db/utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/usr/local/lib/python2.7/site-packages/django/db/backends/utils.py", line 62, in execute
    return self.cursor.execute(sql)
  File "/usr/local/lib/python2.7/site-packages/django/db/backends/mysql/base.py", line 112, in execute
    return self.cursor.execute(query, args)
  File "/usr/local/lib/python2.7/site-packages/MySQLdb/cursors.py", line 226, in execute
    self.errorhandler(self, exc, value)
  File "/usr/local/lib/python2.7/site-packages/MySQLdb/connections.py", line 36, in defaulterrorhandler
    raise errorvalue
django.db.utils.ProgrammingError: (1064, "sphinxql: syntax error, unexpected IDENT, expecting VARIABLES near 'FULL TABLES'")

The patch in #27110 did not help in this case as the error raises in the database introspection.

[Tim Graham]

I'm beginning to think that makemigrations shouldn't have this check in the first place due to the many issues that have been reported (#27054, #27110, #27141) and the more and more complexity that seems needed to add to fix it. Other opinions welcome. In my view, the most natural thing to do after creating a migration is to apply it, so it doesn't seem like delaying the check from makemigrations to migrate would have a huge drawback.

[Shai Berger]

The issue is that makemigrations does not have a target database, while migrate does. So, in order to check for consistent history, makemigrations checks all databases. After this didn't work, the fix was "well, all databases which look like they have migrations", and this still fails.

I see two possible solutions:

1. Use the DB routing mechanism to find the list of databases which can actually be migrated (for the relevant app), and only check consistency in these databases. Then, users with databases which shouldn't be migrated will have an obvious way to protect them.

2. Claim that "makemigrations" shouldn't actually touch the database -- essentially, that it's an operation whose only arguments are the migrations and models -- and accept Tim's suggestion.

It's been a while since I looked at the migration generator, but I suspect that it does ask the database some questions about features etc, which would somewhat violate route 2.

I think makemigrations is a place where we should check consistency, because one of the two possible reasons for inconsistent history is erroneous editing of migration files; creating the next migration on top of an error is something we should prevent if we can, IMO.

I also think just removing the check because of "too many problems", with no principled reasoning behind it, is not a good option, but I won't argue too hard for that; practicality beats purity.

[Tim Graham]

Consulting database routers might work. I think I had rejected that solution in mind my because I thought model_name was a required argument for allow_migrate() but now I see it's not. I'll see if this solution is feasible tomorrow, unless someone works on it or rejects the idea while I'm sleeping.

[Sjoerd Job Postmus]

From my perspective, makemigrations would not really be the place to check database consistency. Especially when trying to build a reusable app, you should not have to know what database connection your app will be used in.

I just tested this (somewhat superficially), but in Django 1.9.7 I can run makemigrations without even having a database connection configured. The minimum requirement seems to be that *if* there is a database configured, the ENGINE field makes sense.

Django 1.10 raises the bar significantly (the bar is being lowered again a bit, which I think is great!)

• All databases you configured need to have a django_migrations table (or the ability to create one). (#27054)
• The django_migrations table needs to be readable. (#27141)
• The configured database must support enough of SQL to make sense of the checks (#27142).
• The data in that table needs to be consistent with your project/app (I expect), even though it might be a read-only connection to a database "owned" by another Django project where you just want to have a "sneak peak" into a specific table. (probably the issue I would have reported if the django_migrations table in #27142 *was* readable).

Sure, consulting database routers might be some form of win, but how much of a win? Will it solve all current problems, including the ones we haven't found yet?

(During some investigation, I also found that Django 1.9.7 already consults the database routers for some model-checks, for instance in _check_long_column_names. If I were to write a reusable app, I think it might be better to check all possible engines, and not just the ones routed to.)

From what I have seen so far, I would kindly request that makemigrations will stop checking database consistency, and make sure that no database queries will be made. I think that would be the most pragmatic choice. To me the question is: will it be removed in the 1.10.x series, or in 1.11?

[[remark: please don't take the above the wrong way. I understand that this feature has been developed based on good intentions, and I really appreciate the effort that has gone into creating and enhancing Django. I am trying not to hurt anyone's feelings and hope the above does not come off as such.]]

[Tim Graham]

On ​an old pull request, Shai said:

The reason makemigrations should fail is that the more likely reason for inconsistent history is a change in the migrations code (in particular, dependencies), rather than direct manipulation of the database. If the code of existing migrations is suspicious, we should avoid building more migrations on it.

I think it's possible to use multiple databases without using database routers. For example, migrate requires a database alias, so if you have a read-only database, it's usable without a router as long as you don't try to migrate it. (I didn't test it so I could be incorrect but perhaps someone can confirm that.)

I think consulting database routers could still be a backwards-incompatible change for some users since it would now require routers to be setup for read-only databases.

[Sjoerd Job Postmus]

How about the intermediate option of adding a --no-check-consistency command line argument.

The benefit of this is that it allows the gradual improvement of the checking algorithm, while still providing an easy to use workaround for when one hits a corner case. Because right now, if you use 1.10, you're blocked for 1.10.1 to fix it.

Then, for 1.11 we can still re-consider the consistency-checking.

[Shai Berger]

Replying to timgraham:

I think it's possible to use multiple databases without using database routers. For example, migrate requires a database alias, so if you have a read-only database, it's usable without a router as long as you don't try to migrate it. (I didn't test it so I could be incorrect but perhaps someone can confirm that.)

I think consulting database routers could still be a backwards-incompatible change for some users since it would now require routers to be setup for read-only databases.

No database routers would mean that, by default, all queries would run against the default database; perhaps in that case we should only check for consistency against it. If the history were consistent in default and inconsistent elsewhere, it would suggest to me that the problem is in that database rather than migration files, and so catching it in migrate would be sufficient.

Perhaps even generally, if default allows migrating the app, there is no need to check for consistency on other databases in makemigrations.

Replying to sjoerdjob:

How about the intermediate option of adding a --no-check-consistency command line argument.

I'm -0 on this as a feature, and -1 on an interim solution. I am neutral 0 on a --no-force-consistency which turns all errors encountered in consistency checking into warnings.

[Tim Graham]

Work in progress ​PR with the database routers concept. Tests yet to be added.

[Tim Graham]

Some tests are added and the patch might be ready for review, however, I think the addition of consistency checks in makemigrations, even with these changes, could remain backwards-incompatible in some edge cases. For example, a project with a read-only database that defines some database routers but hasn't setup a router to prevent migrating on the read-only database will need to add a rule in a router's allow_migrate() to prevent the consistency check. Previously, I think Django would be usable without such a rule (of course, a developer could break things by making a mistake like invoking manage.py migrate --database=readonly_database). I guess anyone in favor of this check in makemigrations would deem the backwards-incompatibility acceptable, provided a note is added in the release notes.

[Markus Gerards]

Replying to timgraham:

Some tests are added and the patch might be ready for review,

This works fine for my case.

Replying to timgraham:

For example, a project with a read-only database that defines some database routers but hasn't setup a router to prevent migrating on the read-only database will need to add a rule in a router's allow_migrate() to prevent the consistency check.

But I believe this is a good solution as you would have most likely set up a database router for your read-only database.

[GitHub <noreply@…>]

In 098c07a:

Fixed #27142, #27110 -- Made makemigrations consistency checks respect database routers.

Partially reverted refs #27054 except for one of the tests as this
solution supersedes that one.

Thanks Shai Berger for the review.

[Tim Graham <timograham@…>]

In 3e913d2:

[1.10.x] Fixed #27142, #27110 -- Made makemigrations consistency checks respect database routers.

Partially reverted refs #27054 except for one of the tests as this
solution supersedes that one.

Thanks Shai Berger for the review.

Backport of 098c07a03286b5ed133102733e67a83061647ea0 from master

[Tim Graham <timograham@…>]

In c93ac9cf:

Refs #25850, #27142, #27110 -- Documented migration history consistency checks.

[Tim Graham <timograham@…>]

In 57f7d896:

[1.10.x] Refs #25850, #27142, #27110 -- Documented migration history consistency checks.

Backport of c93ac9cf42bff259ab71b70a89b693b9c38e4666 from master