Opened 10 years ago
Closed 10 years ago
#22611 closed Bug (wontfix)
sqlclear command tries to drop non-existing constraint with ForeignKey to self (postgresql)
Reported by: | Vidir Valberg Gudmundsson | Owned by: | |
---|---|---|---|
Component: | Core (Management commands) | Version: | 1.7-beta-2 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Having the following model (in an app called test_app):
class Foo(models.Model): bar = models.ForeignKey('self')
The sqlclear
management command returns:
$ ./manage.py sqlclear testapp BEGIN; ALTER TABLE "testapp_foo" DROP CONSTRAINT "bar_id_refs_id_83e148c5"; DROP TABLE "testapp_foo"; COMMIT;
Piping this into psql
results in the following error:
> ./manage.py sqlclear testapp | psql -U django -h localhost django BEGIN ERROR: constraint "bar_id_refs_id_83e148c5" of relation "testapp_foo" does not exist ERROR: current transaction is aborted, commands ignored until end of transaction block ROLLBACK
Checking the table in psql
reveals:
django=# \d testapp_foo Table "public.testapp_foo" Column | Type | Modifiers --------+---------+---------------------------------------------------------- id | integer | not null default nextval('testapp_foo_id_seq'::regclass) bar_id | integer | not null Indexes: "testapp_foo_pkey" PRIMARY KEY, btree (id) "testapp_foo_bar_id" btree (bar_id) Foreign-key constraints: "testapp_foo_bar_id_fkey" FOREIGN KEY (bar_id) REFERENCES testapp_foo(id) DEFERRABLE INITIALLY DEFERRED Referenced by: TABLE "testapp_foo" CONSTRAINT "testapp_foo_bar_id_fkey" FOREIGN KEY (bar_id) REFERENCES testapp_foo(id) DEFERRABLE INITIALLY DEFERRED
So it seems that the sqlclear
command generates a different name for the
constraint than it actually is. I'm guessing that postgresql is in charge of
naming the constraint. At least looking at the output of the sqlall
command, there
is no indication of Django doing any naming.
$ ./manage.py sqlall testapp BEGIN; CREATE TABLE "testapp_foo" ( "id" serial NOT NULL PRIMARY KEY, "bar_id" integer NOT NULL REFERENCES "testapp_foo" ("id") DEFERRABLE INITIALLY DEFERRED ) ; CREATE INDEX "testapp_foo_bar_id" ON "testapp_foo" ("bar_id"); COMMIT;
I've figured out that the "wrongful" naming is done in
django.db.backends.creation.BaseDatabaseCreation.sql_remove_table_constraints
,
but there my knowledge of Djangos ORM stops :)
It might be worth noting that doing a simple DROP statement drops the table
just fine. So maybe the ALTER statement isn't necessary?:
$ echo 'BEGIN; DROP TABLE "testapp_foo"; COMMIT;' | psql -U django -h localhost django BEGIN DROP TABLE COMMIT
(I'm using Django 1.7b3 installed with pip install git+https://github.com/django/django@stable/1.7.x#egg=Django
, but it is not listed in the version list.)
Change History (11)
comment:1 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 10 years ago
Owner: | changed from | to
---|
comment:5 by , 10 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
I've made a pull request (https://github.com/django/django/pull/2659) and would appreciate any feedback. I'm uncertain on a couple of topics:
- The
schema_editor.create_model()
takes care of creating indexes, sosql_indexes()
is not that straight forward to convert to using schema editor. - The old sql functions took the
style
argument, but this is not used anywhere in the schema editor. - The tests pass, but they probably need more improvement.
- Currently it does not do any nice coloring/highlighting of the output. It would be nice, but is not that important in my opinion.
comment:6 by , 10 years ago
Severity: | Normal → Release blocker |
---|
I'd actually like to drop the sql* commands for apps that are marked as having migrations, as they're no longer needed, and we'll finally deprecate and remove them along with other creation-based stuff in 2.0 (they should pretty much be replaced by sqlmigrate, which obviously stays)
In particular, I think that:
- sql
- sqlall
- sqlcustom
- sqlindexes
- sqldropindexes
- sqlsequencereset
should be modified to error and quit if supplied with an app that's got migrations. This would leave just sqlflush
and sqlmigrate
working for those apps, which both make sense in the context.
comment:7 by , 10 years ago
I have made a new PR (now following the contribution guidelines):
https://github.com/django/django/pull/2742
There are still some issues with coloring though. I will address that if the rest of the PR looks good.
comment:8 by , 10 years ago
Severity: | Release blocker → Normal |
---|
valberg has just confirmed on IRC that this is also a problem on 1.6, so as it's no longer a regression, dropping release blocker status.
comment:9 by , 10 years ago
Has patch: | unset |
---|---|
Owner: | removed |
Patch needs improvement: | unset |
Status: | assigned → new |
Since this is an old bug, and the sql commands are likely to go away in future versions due to migrations, I'm stopping the work on this.
comment:11 by , 10 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
I can confirm this bug, I've encountered it while working on #19750. However, the future of 'sql*' management commands is a bit uncertain with the new migration framework. At least, they will need to be updated to use the new
DatabaseSchemaEditor
classes.