Opened 7 months ago

Last modified 3 months 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: master
Severity: Normal Keywords:
Cc: 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 Дилян Палаузов 3 months 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 Дилян Палаузов 3 months ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 months ago by Tim Graham

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 Changed 7 months ago by Дилян Палаузов

Description: modified (diff)

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

Description: modified (diff)

comment:4 Changed 3 months ago by Дилян Палаузов

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 Changed 3 months ago by Tim Graham

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 Changed 3 months ago by Дилян Палаузов

Has patch: set
Version: 1.11master

Changed 3 months ago by Дилян Палаузов

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 Changed 3 months ago by Tim Graham

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).

Changed 3 months ago by Дилян Палаузов

comment:8 Changed 3 months ago by Дилян Палаузов

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).

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