[Django] #27142: makemigrations fails on special database connections
Django
----------------------------+--------------------
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
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 claudep):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:1>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gerricom):
Replying to [comment:1 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:2>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: shaib, MarkusH (added)
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:3>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by shaib):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:4>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:5>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sjoerdjob):
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.]]
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:6>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
On [https://github.com/django/django/pull/5977#discussion_r52391536 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:7>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sjoerdjob):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:8>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by shaib):
Replying to [comment:7 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.
Replying to [comment:8 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:9>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
Work in progress [https://github.com/django/django/pull/7194 PR] with the
database routers concept. Tests yet to be added.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:10>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_tests: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:11>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gerricom):
Replying to [comment:11 timgraham]:
> Some tests are added and the patch might be ready for review,
This works fine for my case.
Replying to [comment:11 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:12>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"098c07a03286b5ed133102733e67a83061647ea0" 098c07a]:
{{{
#!CommitTicketReference repository=""
revision="098c07a03286b5ed133102733e67a83061647ea0"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:13>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"3e913d214133ca92329ed95d55bd881e42d487ef" 3e913d2]:
{{{
#!CommitTicketReference repository=""
revision="3e913d214133ca92329ed95d55bd881e42d487ef"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:14>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"c93ac9cf42bff259ab71b70a89b693b9c38e4666" c93ac9cf]:
{{{
#!CommitTicketReference repository=""
revision="c93ac9cf42bff259ab71b70a89b693b9c38e4666"
Refs #25850, #27142, #27110 -- Documented migration history consistency
checks.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:15>
Django
Reporter: gerricom | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"57f7d8968d8443352ce331ad9dd6f433a15d0d93" 57f7d896]:
{{{
#!CommitTicketReference repository=""
revision="57f7d8968d8443352ce331ad9dd6f433a15d0d93"
[1.10.x] Refs #25850, #27142, #27110 -- Documented migration history
consistency checks.
Backport of c93ac9cf42bff259ab71b70a89b693b9c38e4666 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27142#comment:16>