Opened 7 years ago

Last modified 5 weeks ago

#28715 new Cleanup/optimization

Prevent a migration changing DateTimeField(auto_now_add=True) to default=timezone.now from generating SQL

Reported by: Дилян Палаузов Owned by: nobody
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Ülgen Sarıkavak Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Дилян Палаузов)

A switch from DateTimeField(auto_now_add=True) to DateTimeField(default=django.utils.timezone.new) creates the statements

ALTER TABLE SET DEFAULT '2017-10-16T09:35:52.710695'::timestamp;
ALTER TABLE DROP DEFAULT

which have no effects, apart from locking the whole table twice.

A proposal to recognize, when the effective default-callable doesn't change and skip changing the DEFAULT twice in this case, as well as not generating a migration when this is the only change on a field:

diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py
--- a/django/db/backends/base/schema.py
+++ b/django/db/backends/base/schema.py
@@ -199,28 +199,33 @@ class BaseDatabaseSchemaEditor(object):
             'requires_literal_defaults must provide a prepare_default() method'
         )
 
-    def effective_default(self, field):
+    @staticmethod
+    def effective_default_before_callable(field):
         """
-        Returns a field's effective database default value
+        Returns a field's effective database default callable or value
         """
         if field.has_default():
-            default = field.get_default()
+            return field._get_default
         elif not field.null and field.blank and field.empty_strings_allowed:
             if field.get_internal_type() == "BinaryField":
-                default = six.binary_type()
+                return six.binary_type()
             else:
-                default = six.text_type()
+                return six.text_type()
         elif getattr(field, 'auto_now', False) or getattr(field, 'auto_now_add', False):
             default = datetime.now()
             internal_type = field.get_internal_type()
             if internal_type == 'DateField':
-                default = default.date
+                return default.date
             elif internal_type == 'TimeField':
-                default = default.time
+                return default.time
             elif internal_type == 'DateTimeField':
-                default = timezone.now
-        else:
-            default = None
+                return timezone.now
+
+    def effective_default(self, field):
+        """
+        Returns a field's effective database default value
+        """
+        default = BaseDatabaseSchemaEditor.effective_default_before_callable(field)
         # If it's a callable, call it
         if callable(default):
             default = default()
@@ -615,6 +620,7 @@ class BaseDatabaseSchemaEditor(object):
             old_default != new_default and
             new_default is not None and
             not self.skip_default(new_field)
+            and BaseDatabaseSchemaEditor.effective_default_before_callable(old_field) != BaseDatabaseSchemaEdit
         )
         if needs_database_default:
             if self.connection.features.requires_literal_defaults:
diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py
index 8d40c77..2f5d5c2 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -1232,7 +1232,7 @@ class DateField(DateTimeCheckMixin, Field):
         if self.auto_now:
             kwargs['auto_now'] = True
         if self.auto_now_add:
-            kwargs['auto_now_add'] = True
+            kwargs['default'] = datetime.date.today
         if self.auto_now or self.auto_now_add:
             del kwargs['editable']
             del kwargs['blank']
@@ -1372,6 +1372,12 @@ class DateTimeField(DateField):
 
         return []
 
+    def deconstruct(self):
+        name, path, args, kwargs = super(DateTimeField, self).deconstruct()
+        if self.auto_now_add:
+            kwargs['default'] = timezone.now
+        return name, path, args, kwargs
+
     def get_internal_type(self):
         return "DateTimeField"

Attachments (2)

auto_now_add_skip_sql_calls.patch (11.8 KB ) - added by Дилян Палаузов 6 years ago.
Probably somebody with more experience will tweak the test.migrations.test_autodetector.test_add_date_fields_with_auto_now_add_(not_asking_for_null_addition, add_asking_for_default) tests, so that the line "kwargsauto_now_add = True" in django/db/models/fields/init.py can be removed. But otherwise this shall be ready to go.
auto_now_add_skip_sql_calls.2.patch (11.3 KB ) - added by Дилян Палаузов 6 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by Tim Graham, 7 years ago

Summary: Migration DateTimeField(auto_now_add=True -> default=django.utils.timezone.new)Prevent a migration changing DateTimeField(auto_now_add=True) to default=timezone.now from generating SQL
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:2 by Дилян Палаузов, 7 years ago

Description: modified (diff)

comment:3 by Дилян Палаузов, 7 years ago

Description: modified (diff)

comment:4 by Дилян Палаузов, 6 years ago

My plan is to work one more month with Django, so if there are concerns on this patch, please raise them by then.

comment:5 by Tim Graham, 6 years ago

The patch must apply cleanly to Django's master branch and it must have a test. Ideally, you could provide the patch as a pull request and then check "Has patch" on the ticket so that it appears in the review queue.

comment:6 by Дилян Палаузов, 6 years ago

Has patch: set
Version: 1.11master

by Дилян Палаузов, 6 years ago

Probably somebody with more experience will tweak the test.migrations.test_autodetector.test_add_date_fields_with_auto_now_add_(not_asking_for_null_addition, add_asking_for_default) tests, so that the line "kwargsauto_now_add = True" in django/db/models/fields/init.py can be removed. But otherwise this shall be ready to go.

comment:7 by Tim Graham, 6 years ago

Patch needs improvement: set

Sorry, I should have looked at the initial patch before I suggested bringing it up to date. I don't think the approach is correct. I think that a migration should be generated if auto_now_add=True changes to default=timezone.now as they aren't entirely equivalent. The fix will have to be at the SchemaEditor level. Given that auto_now_add and auto_now might be deprecated (#22995), it might be a better use of time to focus on that issue (although the patch forward isn't entirely clear).

by Дилян Палаузов, 6 years ago

comment:8 by Дилян Палаузов, 6 years ago

The purpose of the change is to tweak the migrations auto_now_add -> default for DateField and DateTimeField not to call

ALTER TABLE SET DEFAULT '2017-10-16T09:35:52.710695'::timestamp;
ALTER TABLE DROP DEFAULT

towards the database. The attached auto_now_add_skip_sql_calls.2.patch touches BaseDatabaseSchemaEditor, and does generate a migration, which however skips modifying the DEFAULT.

auto_now_add being possibly deprecated means that this type of migration will be created more often in the future and this patch accelerates the execution of the migration (as it does not lock the table).

comment:9 by Ülgen Sarıkavak, 5 weeks ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top