Opened 8 years ago

Closed 8 years ago

Last modified 2 years ago

#28000 closed Cleanup/optimization (duplicate)

Avoid SET/DROP DEFAULT unless a field changes from null to non-null

Reported by: Matteo Pietro Russo Owned by: nobody
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: 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 Matteo Pietro Russo)

If i consider to modify a column (ChaField) to add a default value as follow:

from django.db import models

class PersonModel(models.Model):
    nickname = models.CharField(default="noname", max_length=100, null=False)

makemigration insists on creating a new migration for this change. However, the migration on MYSQL shows:

BEGIN;
ALTER TABLE "core_personmodel" ALTER "nickname" SET DEFAULT 'noname';
ALTER TABLE "core_personmodel" ALTER "nickname" DROP DEFAULT;
COMMIT;

I think there is a mistake at line 735 in django/db/backends/base/schema.py

        # Drop the default if we need to
        # (Django usually does not use in-database defaults)
        if needs_database_default:
            sql = self.sql_alter_column % {
                "table": self.quote_name(model._meta.db_table),
                "changes": self.sql_alter_column_no_default % {
                    "column": self.quote_name(new_field.column),
                }
            }
            self.execute(sql)

If I "needs_database_default", why we need to drop default (sql_alter_column_no_default)? I read we use it to prevent extraneous DROP DEFAULT statements (an example is LONGTEXT in MySQL) because if database is able to use default we need to avoid to drop it.

Change History (15)

comment:1 by Matteo Pietro Russo, 8 years ago

Description: modified (diff)

comment:2 by Tim Graham, 8 years ago

Resolution: invalid
Status: newclosed

Django uses database defaults to set values on existing rows in a table. It doesn't leave the default values in the database so dropping the default is correct behavior. There might be an optimization not to set/drop the default in this case -- I'm not sure it's needed since the column isn't null. A separate ticket could be opened for this.

comment:3 by Matteo Pietro Russo, 8 years ago

Resolution: invalid
Status: closednew

About default value, I read it in Django documentation:

"The default value is used when new model instances are created and a value isn’t provided for the field."

It's right if I use Django ORM, but I expect my field has rule I set in my model.

You said that "Django uses database defaults to set values on existing rows in a table", but if I migrate my table adding default value, migration doesn't change anything: It doesn't set default value on existing rows, it' doesn't change field (it is nullable yet and i haven't default).

from django.db import models

class PersonModel(models.Model):
    nickname = models.CharField(max_length=100, null=True)

If I change it:

    nickname = models.CharField(default="noname", max_length=100, null=False)

makemigrations builds a migration with this operation:

        migrations.AlterField(
            model_name='core_personmodel',
            name='nickname',
            field=models.CharField(default="noname", max_length=100, null=False),
        ),

If I execute migration:

Running migrations:
  Rendering model states... DONE
  Applying core.0004_migration... OK

but It's doesn't do anything.

Version 0, edited 8 years ago by Matteo Pietro Russo (next)

comment:4 by Tim Graham, 8 years ago

Resolution: invalid
Status: newclosed

This is expected behavior. I should have said, "Django uses database defaults to set values on existing *null* rows in a table."

comment:5 by Matteo Pietro Russo, 8 years ago

Ok, now I understand. For this reason, It's not a bug.

So I can't set default value on database via Django migrations, can I?

Default values are used by Django ORM, I think it's an useful improvement if we can apply it in db via migrations (as soon as possible). Do you agree?

MySQL backend uses base alter_field method so I can override it as proposal.

comment:7 by Tim Graham, 8 years ago

No, it's not acceptable to overwrite existing data the default without the user's consent. You should use a RunPython operation to do that if needed.

comment:8 by Matteo Pietro Russo, 8 years ago

I don't want to overwrite anything: i want to set default value in my db column (also if my table is empty).

ALTER TABLE "core_personmodel" ALTER "nickname" SET DEFAULT 'noname';

This statement doesn't modify data but column's specification, If I run it and then I run django migration, It drops my previous SQL: this is a mistake because I lost my defaults.

Why cannot I do that?

comment:9 by Simon Charette, 8 years ago

This statement doesn't modify data but column's specification, If I run it and then I run django migration, It drops my previous SQL: this is a mistake because I lost my defaults.
Why cannot I do that?

Django's ORM don't use defaults defined at the database level and always specify the value when performing inserts instead.

There have been discussions to add support for database generated field values but nothing have been implemented so far.

in reply to:  9 comment:10 by Matteo Pietro Russo, 8 years ago

Replying to Simon Charette:

This statement doesn't modify data but column's specification, If I run it and then I run django migration, It drops my previous SQL: this is a mistake because I lost my defaults.
Why cannot I do that?

Django's ORM don't use defaults defined at the database level and always specify the value when performing inserts instead.

There have been discussions to add support for database generated field values but nothing have been implemented so far.

This behaviour is correct but, I expect django migration doesn't delete my column specification (it DROPS my defaults value).

comment:11 by Simon Charette, 8 years ago

Has patch: set
Summary: Migration forces "DROP DEFAULT" SQLAvoid SET/DROP DEFAULT on field alteration if not required.
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 1.9master

Tim, I'll just repurpose this ticket for the optmization case.

PR.

comment:12 by Tim Graham, 8 years ago

Resolution: invalid
Status: closednew
Summary: Avoid SET/DROP DEFAULT on field alteration if not required.Avoid SET/DROP DEFAULT unless a field changes from null to non-null

comment:13 by Simon Charette, 8 years ago

Resolution: duplicate
Status: newclosed

This is duplicate of #27928.

comment:14 by Marcin Nowak, 7 years ago

Django's ORM don't use defaults defined at the database level

But it should.

in reply to:  14 comment:15 by Alessio Fachechi, 2 years ago

Replying to Marcin Nowak:

Django's ORM don't use defaults defined at the database level

But it should.

Totally agree. What's the reason for dropping them? I can understand the benefits of making Django handle its own default logic, but that doesn't mean dropping the database default, which can be used at last stage. Also the Django app is not always the only app accessing and writing to a database...

comment:16 by Tim Graham, 2 years ago

Leaving database defaults would be backward incompatible and could result in surprising behavior. Field.default can be a callable which returns a non-static value that could be inappropriate as a long-running default.

#470 is a new feature ticket to allow using database defaults.

Note: See TracTickets for help on using tickets.
Back to Top