Re: [Django] #36170: Running no-op migrations (verbose_name, ...) slow on sqlite

[Django]

#36170: Running no-op migrations (verbose_name, ...) slow on sqlite
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Klaas van Schelven):

* component: Uncategorized => Migrations
* type: Uncategorized => Cleanup/optimization

--
Ticket URL: <https://code.djangoproject.com/ticket/36170#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36170: Running no-op migrations (verbose_name, ...) slow on sqlite
-------------------------------------+-------------------------------------

Reporter: Klaas van Schelven | Type:
| Uncategorized
Status: new | Component:
| Uncategorized
Version: 5.1 | Severity: Normal

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Running no-op migrations (verbose_name, help_text, choices) triggers DB-
wide check_constraints on sqlite

This is because
[https://github.com/django/django/blob/5da3ad7bf90fba7321f4c2834db44aa920c70bc7/django/db/backends/sqlite3/schema.py#L39
this is done as part of the DatabaseSchemaEditor's exit] which is
[https://github.com/django/django/blob/5da3ad7bf90fba7321f4c2834db44aa920c70bc7/django/db/migrations/executor.py#L252
unconditionally called as part of apply_migration].

Because [https://code.djangoproject.com/ticket/21498 no-op migrations
cannot be ignored by design] (also
[https://code.djangoproject.com/ticket/30048 here]) this means that
changing your help-text will lead to an unnecessary check of all
constraints in the database (if no workarounds are employed, e.g.
squashing with something else).

If we accept the principle of "no ignoring of fields on-detect" as per the
issues mentioned above, we might still try to detect the fact that for any
given field and DB nothing happened in practice (and hence no
check_constraints is needed)

In particular: the call to `check_constraints` was introduced as part of
[https://github.com/django/django/commit/7289874adceec46b5367ec3157cdd10c711253a0
this commit], in which it is directly tied to a 5-step process.

Without claiming to have fully thought through all consequences: can't we
pull the check closer to those 5 steps, or make it conditional to
`_remake_table` having been actually called between enter & exit?

**Refs / background:**

I'm doing this in the context of [https://www.bugsink.com/ Bugsink, a
Self-hosted Error Tracker]; SQLite is a first level citizen (for
production) in that tool, which means large databases can be expected. On
a not all too big ~100K-row database I timed the above unnecessary check
to take ~20 seconds. Scaling that up by 1 or 2 orders of magnitude would
be my goal. Charitably assuming linear growth in time-complexity, you can
see how this can quickly become ''really annoying.''

[https://stackoverflow.com/a/39801321/339144 this SO post] provides tricks
for avoiding the creation of such migrations in the first place (may be
relevant context as long as the above isn't fixed yet).
--
Ticket URL: <https://code.djangoproject.com/ticket/36170>

[Django]

#36170: Running no-op migrations (verbose_name, ...) slow on sqlite

--------------------------------------+------------------------------------

Reporter: Klaas van Schelven | Owner: (none)

Type: Cleanup/optimization | Status: new

Component: Migrations | Version: 5.1
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Simon Charette):

* stage: Unreviewed => Accepted

Comment:

Interesting use case. I think it's worth a shot but we should definitely
error on the side of caution here as to avoid causing subtle integrity
violation regressions. That is the `PRAGMA foreign_key_check` might be
necessary for other operations that SQLite added support for over the
years such as `ADD FIELD` assuming it's a foreign key that can be
populated with invalid data.

I believe the following should cover all no-op migrations while avoiding
the daunting task of figuring out for which operations are safe to skip
foreign key checks for

{{{#!diff
diff --git a/django/db/backends/sqlite3/schema.py
b/django/db/backends/sqlite3/schema.py
index 3617d17fac..a74c010384 100644
--- a/django/db/backends/sqlite3/schema.py
+++ b/django/db/backends/sqlite3/schema.py
@@ -33,13 +33,23 @@ def __enter__(self):
"SQLite does not support disabling them in the middle of
"
"a multi-statement transaction."
)
+ # Avoid checking foreign key constraint for no-op migrations that
+ # don't execute SQL as it can be expensive on large databases.
+ self.must_check_constraints = False
return super().__enter__()

def __exit__(self, exc_type, exc_value, traceback):
- self.connection.check_constraints()
+ if self.must_check_constraints:
+ self.connection.check_constraints()
super().__exit__(exc_type, exc_value, traceback)
self.connection.enable_constraint_checking()

+ def execute(self, *args, **kwargs):
+ # Execution of any SQL results in a subsequent foreign key
constraint
+ # check to account for them being disabled during schema
alteration.
+ self.must_check_constraints = True
+ return super().execute(*args, **kwargs)
+
def quote_value(self, value):
# The backend "mostly works" without this function and there are
use
# cases for compiling Python without the sqlite3 libraries (e.g.
}}}

Does that sound like a reasonable compromise?
--
Ticket URL: <https://code.djangoproject.com/ticket/36170#comment:2>

[Django]

#36170: Running no-op migrations (verbose_name, ...) slow on sqlite
--------------------------------------+------------------------------------
Reporter: Klaas van Schelven | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by Klaas van Schelven):

I'm not sure I'd call it a "compromise", or simply a "solution" :-)

A quick test on both a real migration and a non-DB-altering one shows that
this diff has the desired effect.
--
Ticket URL: <https://code.djangoproject.com/ticket/36170#comment:3>

[Django]

#36170: Running no-op migrations (verbose_name, ...) slow on sqlite

-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner: Ahmed
Type: | Nassar
Cleanup/optimization | Status: assigned

Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Ahmed Nassar):

* owner: (none) => Ahmed Nassar
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36170#comment:4>

[Django]

#36170: Running no-op migrations (verbose_name, ...) slow on sqlite
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner: Ahmed
Type: | Nassar
Cleanup/optimization | Status: assigned
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Ahmed Nassar):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36170#comment:5>

[Django]

#36170: Running no-op migrations (verbose_name, ...) slow on sqlite
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner: Ahmed
Type: | Nassar
Cleanup/optimization | Status: assigned
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Ahmed Nassar):

I have Optimized **no-op migration** performance on `SQLite`

- Introduced `must_check_constraints` in `DatabaseSchemaEditor` to track
whether constraints should be checked.
- Ensured that `check_constraints()` is only triggered when SQL statements
are executed.
- Added tests to validate the behavior across different migration
scenarios.

PR: https://github.com/django/django/pull/19278
--
Ticket URL: <https://code.djangoproject.com/ticket/36170#comment:6>

[Django]

#36170: Running no-op migrations (verbose_name, ...) slow on sqlite
-------------------------------------+-------------------------------------
Reporter: Klaas van Schelven | Owner: Ahmed
Type: | Nassar
Cleanup/optimization | Status: assigned
Component: Migrations | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by David Smith):

* needs_better_patch: 0 => 1
* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36170#comment:7>