RemoveField dropping Views without notice

Django Internals ORM
1

Hi, could it be possible to remove cascade from delete SQLs/operations: django/django/db/backends/base/schema.py at 5.0.6 · django/django · GitHub? I couldn’t find any information about need of having it there.

Our team was struggling long time recently to find out which cause missing views in our production DB. And it looks like we are not alone: Doc'd that PostgreSQL specifies CASCADE when dropping columns. by shangxiao · Pull Request #18066 · django/django · GitHub.

IMO it would be safer to cause error while applying the migration rather than ending in unexpected state of DB. Developer would know all consequences about RemoveField operation in advance and could take appropriate countermeasures to apply this operation without side effects.

1 Like
2

Hello there, i just want to contribute to this, not having a solid opinion on it.

I believe that the problem is that, most of these side effects will be only knew about on production, taking the scenario of having a view on your production database, is likely that the developer won’t have any views on his local database. Making this really hard to spot, even if you’re removing the CASCADE from the operation.

If you really want to strict this, you can always monkey patch the SQL string on the schema class anyway.

3

I think that it is good practise to have views in migrations as well. So they should be on your local DB too and cause error while developing or in CI test phase.

Otherwise, it could fall at least in staging environment while applying migrations. Or, in the last, in production environment where could be better to stop migrations with side effects rather than have missing of half of the views without knowing what the hack has just happened :slight_smile:

4

@petrprikryl I think that preventing database objects not managed by Django from being deleted is a good enough reason to revisit the inclusion of CASCADE.

My concerns though are that it’s not as simple as removing the suffix and calling it a day, there is likely a reason why it’s being used. Did you do a bit of Git-archeology through git-blame or tried running the test suite to understand why it’s being used?

Understanding why it’s the case and what it can be replaced with would make for a stronger and better informed case than solely reporting the problem it poses to your workflow.

2 Likes
5

I tried to find why it is in Django before sending the post. But only found that it is there from first commit of schema alteration without any more information:

I believe that it is ispired by south where it is too:

Drop table now cascades by default, as this is usually the intended behaviour. You can pass cascade=False to stop this.

I didn’t tried to run test suit as I wanted to get some more information and opinion from community about this problem first. Maybe @andrewgodwin could bring some light here.

6

So I found out that one use case for cascade on sql_delete_table is automatic dropping of constraints.

django-docker-box for postgres after removing cascade:

ERROR [0.000s]: test_unsupported_transactional_ddl_disallowed (schema.tests.SchemaTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tests/django/django/db/backends/utils.py", line 103, in _execute
    return self.cursor.execute(sql)
  File "/usr/local/lib/python3.10/site-packages/psycopg/cursor.py", line 732, in execute
    raise ex.with_traceback(None)
psycopg.errors.DependentObjectsStillExist: cannot drop table schema_authorwithevenlongername because other objects depend on it
DETAIL:  constraint schema_bookwithlongn_author_foreign_key_w_14c57257_fk_schema_au on table schema_bookwithlongname depends on table schema_authorwithevenlongername
constraint schema_bookwithlongn_author_other_really__6c36f4d2_fk_schema_au on table schema_bookwithlongname depends on table schema_authorwithevenlongername
HINT:  Use DROP ... CASCADE to drop the dependent objects too.

So constraints would have to be removed manually. Maybe with help of pg_get_constraintdef postgresql - Retrieving all PK and FK - Database Administrators Stack Exchange. But it starting to look like too postgres specific and I am not sure if there is any interest for this change. I’m looking forward to hearing your opinions.

Removing cascade on sql_delete_column seems to have no impact on test suite for postgres, sqlite and mariadb databases.

7

My gut reaction is that the CASCADE option is the most sensible default.

In addition to views, constraints and the foreign key references, there could also be indexes, formulas for calculated columns, functions, procedures, and triggers to name a few - all of which may need to be deleted when a column or table is dropped.

8

Next argument for removing the cascade is that the only backend that uses it is postgres. So the behaviour is not consistent across backends:

Next, Django is dropping the constraints explicitly. So cascade shouldn’t be needed. It explains why my test suite was passing after removing cascade from sql_delete_column (my previous post).

9

Thanks for all the sleuthing work @petrprikryl, this is great.

Thinking more of it I suspect that one of the reason why CASCADE was used only of Postgres was to make sure not to leave orphaned sequences attached to serial fields which Django use to rely on for its AutoField before moving to GENERATED BY DEFAULT AS IDENTITY in Fixed #30511 -- Used identity columns instead of serials on PostgreSQL. · django/django@2eea361 · GitHub.

Given we now explicitly delete them it might effectively not be necessary anymore.

1 Like
10

Thank you @charettes for keeping the pace.

I found out that old sequences are covered by:

So you have right.

Do you think that we can proceed to ticket phase?

1 Like
11

I would say yes, you have a solid case here.

3 Likes
12

After reading the thread, I agree. Using CASCADE can only do more harm than good, now we know why Chesterton put the fence there.

1 Like
13

Just for reference: #35487 (Removal of CASCADE in DROP COLUMN migration operation) – Django