[Django] #31765: schema.tests.SchemaTests.test_db_table fails on MacOS
Django
-----------------------------------------+------------------------
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
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 |
-----------------------------------------+------------------------
I can reproduce this failure on master with the latest Python 3.7 and 3.8,
with sqlite3 version `3.28.0`:
{{{
======================================================================
FAIL: test_db_table (schema.tests.SchemaTests)
Tests renaming of the table
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/Users/tom/PycharmProjects/github/orf/django/django/test/utils.py", line
381, in inner
return func(*args, **kwargs)
File
"/Users/tom/PycharmProjects/github/orf/django/tests/schema/tests.py", line
2280, in test_db_table
self.assertForeignKeyExists(Book, "author_id", "schema_otherauthor")
File
"/Users/tom/PycharmProjects/github/orf/django/tests/schema/tests.py", line
201, in assertForeignKeyExists
self.assertEqual(constraint_fk, (expected_fk_table, field))
AssertionError: Tuples differ: ('schema_author', 'id') !=
('schema_otherauthor', 'id')
First differing element 0:
'schema_author'
'schema_otherauthor'
- ('schema_author', 'id')
+ ('schema_otherauthor', 'id')
? +++++
----------------------------------------------------------------------
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31765>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tom Forbes):
It seems that the `sql` column of the `sqlite_master` isn't updated, which
seems to be related to the `supports_atomic_references_rename` feature
being set to True. Setting to to False makes the tests pass. Below is the
`sqlite_master` output for the test:
{{{
('table', 'schema_otherauthor', 'schema_otherauthor', 33, 'CREATE TABLE
"schema_otherauthor" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT,
"name" varchar(255) NOT NULL)')
('table', 'schema_book', 'schema_book', 36, 'CREATE TABLE "schema_book"
("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "author_id" integer NOT
NULL REFERENCES "schema_author" ("id") DEFERRABLE INITIALLY DEFERRED)')
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:1>
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
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 Tom Forbes):
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Tested with different versions of SQLite on python 3.8. It seems that
specifically the SQLite version that's bundled with MacOS (3.28.0) fails,
whereas if you install 3.28.0 via Homebrew it passes.
I have a patch here to switch to feature detection:
https://github.com/django/django/pull/13153. It's not nice, but there is
no other way I can find to detect a "MacOS" sqlite vs another SQLite.
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:2>
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
I cannot come with an alternative proposal, but it's definitely not nice,
as you are putting some load on everyone because of a bug in one version
of one distribution (which is probably not a common production platform).
What about the documentation way?
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:3>
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tom Forbes):
I think our saving grace here is that "supports_atomic_references_rename"
should only be invoked during the migrations path. I'm not sure that this
would ever be invoked during startup, and so shouldn't add any latency?
Can you elaborate on the documentation route? My main rationale with
fixing this was to get the test suite to pass, so I can progress with
helping with the GSOC branch.
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:4>
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> .. but there is no other way I can find to detect a "MacOS" sqlite vs
another SQLite.
@Tom: can you try `PRAGMA compile_options;` against the two versions? (If
we can see why the difference is then...) 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:5>
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tom Forbes):
This is the output from the `sqlite3` CLI tool, running the MacOS sqlite3:
{{{
BUG_COMPATIBLE_20160819
COMPILER=clang-11.0.3
DEFAULT_CACHE_SIZE=2000
DEFAULT_CKPTFULLFSYNC
DEFAULT_JOURNAL_SIZE_LIMIT=32768
DEFAULT_PAGE_SIZE=4096
DEFAULT_SYNCHRONOUS=2
DEFAULT_WAL_SYNCHRONOUS=1
ENABLE_API_ARMOR
ENABLE_COLUMN_METADATA
ENABLE_DBSTAT_VTAB
ENABLE_FTS3
ENABLE_FTS3_PARENTHESIS
ENABLE_FTS3_TOKENIZER
ENABLE_FTS4
ENABLE_FTS5
ENABLE_JSON1
ENABLE_LOCKING_STYLE=1
ENABLE_PREUPDATE_HOOK
ENABLE_RTREE
ENABLE_SESSION
ENABLE_SNAPSHOT
ENABLE_SQLLOG
ENABLE_UNKNOWN_SQL_FUNCTION
ENABLE_UPDATE_DELETE_LIMIT
HAVE_ISNAN
MAX_LENGTH=2147483645
MAX_MMAP_SIZE=1073741824
MAX_VARIABLE_NUMBER=500000
OMIT_AUTORESET
OMIT_LOAD_EXTENSION
STMTJRNL_SPILL=131072
THREADSAFE=2
USE_URI
}}}
I can't see anything there that would be suitable to to safely detect
MacOS bundled SQLite. We could maybe branch off "COMPILER" and maybe
"BUG_COMPATIBLE_20160819" but it seems equally as dirty.
So, just to re-iterate the point above: This code path should only be
called during migrations. While feature detection is definitely slower, in
this case I feel that it's warranted - looking through the git history for
this specific flag the version range has been changed previously, and it's
not in the hot path of any requests and shouldn't slow down startup.
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:6>
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
What about Simon's suggestion on the pull request?
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:7>
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tom Forbes):
Oops, I missed that comment. I left a reply. I'm happy to go with the
crowd and implement that suggestion if that's the general consensus and
that enables this to be merged, but I can't help but feel it's not the
right path.
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:8>
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Tom. Sorry if I'm being a bit slow...
> I can't see anything there that would be suitable to to safely detect
MacOS bundled SQLite.
I haven't got the exact reproduce in front of me but, if there's not a
compile flag difference, then (surely) this is some bug in the macOS
shipped version of SQLite?
(I'm not seeing the Why? of Why this bug comes up?)
If so, I'm not at all sure that we should add too much code to work around
it. (Define "too much", but is this even our issue?)
It's the sort of thing I might say to add a note to
[https://docs.djangoproject.com/en/3.0/internals/contributing/writing-code
/unit-tests/#troubleshooting the Troublesooting section in the Running the
unit tests guide], saying "Update SQLite, preferably from homebrew" or
similar. SQLite is already on 3.32... — how long is any code we're going
add here be operational?
Again, sorry if I'm just not seeing it.
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:9>
Django
Reporter: Tom Forbes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tom Forbes):
Replying to [comment:9 Carlton Gibson]:
> It's the sort of thing I might say to add a note to
[https://docs.djangoproject.com/en/3.0/internals/contributing/writing-code
/unit-tests/#troubleshooting the Troublesooting section in the Running the
unit tests guide], saying "Update SQLite, preferably from homebrew" or
similar. SQLite is already on 3.32... — how long is any code we're going
add here be operational?
>
> Again, sorry if I'm just not seeing it. (I'm not at all sure what the
best approach here is.)
Part of the problem is that it's not as simple as just updating SQLite
from Homebrew, you also have to set specific `LDFLAGS` and `CPPFLAGS`
environment variables when building Python. I'm not sure many people would
do this (I have never done it previously). So while we could fix it in the
docs but I think that it would be confusing to a lot of people.
I've adapted my PR to exclude this feature when using SQLite 3.28.0 on
MacOS 10.15.x which I think is a good fix. As you said, sqlite is on
3.32.3 now, so it's highly likely that 3.28.0 on MacOS is always the
bundled version.
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:10>
Django
Reporter: Tom Forbes | Owner: Tom Forbes
Type: Bug | Status: assigned
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tom Forbes):
* owner: nobody => Tom Forbes
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:11>
Django
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:12>
Django
Reporter: Tom Forbes | Owner: Tom
| Forbes
Component: Uncategorized | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"80a8be03d9321669a239dbced8ac48a4ebbbb7e1" 80a8be0]:
{{{
#!CommitTicketReference repository=""
revision="80a8be03d9321669a239dbced8ac48a4ebbbb7e1"
Fixed #31765 -- Disabled bundled SQLite renaming atomic references on
macOS 10.15.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:13>
Django
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Skylude):
* needs_better_patch: 0 => 1
Comment:
Seems to still be an issue with MacOSX 11 and SQLite 3.32
>>> platform.version()
'Darwin Kernel Version 20.2.0: Wed Dec 2 20:39:59 PST 2020;
root:xnu-7195.60.75~1/RELEASE_X86_64'
$ python -c "import sqlite3; print(sqlite3.sqlite_version)"
3.32.3
I've installed sqlite from homebrew and have the latest. It is a new
computer. Seems to only happen with Python 3.9.1 (3.9.0, 3.8.4, 3.7.7 all
worked for me).
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:14>
Django
Reporter: Tom Forbes | Owner: Tom
| Forbes
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: closed => new
* resolution: fixed =>
Comment:
OK, this isn't fixed. Reproduces with this environment:
{{{
>>> import platform
>>> import sqlite3
>>> platform.mac_ver()
('10.16', ('', '', ''), 'x86_64')
>>> sqlite3.sqlite_version_info
(3, 32, 3)
}}}
In the source we limited the check to `if
platform.mac_ver()[0].startswith('10.15.') and
Database.sqlite_version_info == (3, 28, 0):`
See 80a8be03d9321669a239dbced8ac48a4ebbbb7e1
We were hoping it was just the single version. There must be something
about the macOS build that distinguishes it from others, if we can just
identify that. 🧐
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:15>
Django
Reporter: Tom Forbes | Owner: Tom Forbes
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:16>
Django
Reporter: Tom Forbes | Owner: Tom Forbes
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
OK, I found it. It's to do with the `legacy_alter_table` config option.
([https://www.sqlite.org/pragma.html#pragma_legacy_alter_table docs]):
{{{
sqlite> .dbconfig
...
legacy_alter_table on
...
sqlite> PRAGMA legacy_alter_table = off;
sqlite> .dbconfig
...
legacy_alter_table off
...
sqlite> CREATE TABLE ATOMIC_RENAME_TEST(id INT);
sqlite> CREATE TABLE ATOMIC_RENAME_TEST_2(one INT, FOREIGN KEY(one)
REFERENCES ATOMIC_RENAME_TEST(id));
sqlite> ALTER TABLE ATOMIC_RENAME_TEST RENAME TO ATOMIC_RENAME_TEST_3;
sqlite> .schema
CREATE TABLE IF NOT EXISTS "ATOMIC_RENAME_TEST_3"(id INT);
CREATE TABLE ATOMIC_RENAME_TEST_2(one INT, FOREIGN KEY(one) REFERENCES
"ATOMIC_RENAME_TEST_3"(id));
}}}
Not sure (yet) why (or where) this defaults to `on` for macOS but we can
configure the connection on creation.
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:17>
Django
Reporter: Tom Forbes | Owner: Tom
| Forbes
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* component: Uncategorized => Database layer (models, ORM)
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:18>
Django
Reporter: Tom Forbes | Owner: Tom
| Forbes
Component: Database layer | Version: dev
(models, ORM) |
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 GitHub <noreply@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"063cf98d3a6839f40c423cbd845def429c5cf0ce" 063cf98]:
{{{
#!CommitTicketReference repository=""
revision="063cf98d3a6839f40c423cbd845def429c5cf0ce"
Fixed #31765 -- Enforced enhanced ALTER TABLE behavior for SQLite
connections.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:19>
Django
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andy Chosak):
Unfortunately the unit test failure described above is pointing out that
`RenameModel` migrations won't properly repoint foreign keys when run
against certain versions of SQLite on MacOS. This interferes with testing
and developing using model renames on MacOS with currently supported
versions of Django -- please see
https://github.com/wagtail/wagtail/issues/8635 for a specific example.
I see this error (both the unit test failure and the incorrect model
rename behavior) on MacOS 11.6.5 with its default SQLite3 version 3.32.3.
Digging into the (long!) history of Django changes to support SQLite
`ALTER TABLE` statements, the relevant technical detail here seem to be my
SQLite pragma defaults of `foreign_keys=0` and `legacy_alter_table=1`.
Per the SQLite docs
[https://www.sqlite.org/draft/lang_altertable.html#alter_table_rename
here], these pragma values mean that parent table references are **not**
automatically updated.
This issue first came up in #29182, which resulted in a proposed fix in
53b17d4734f06372b66e3ac4db7a1740c503b330.
This was then updated in #30033 to try to comply with an alternate
procedure for `ALTER TABLE` documented
[https://www.sqlite.org/lang_altertable.html#otheralter here]. This was
implemented in 7289874adceec46b5367ec3157cdd10c711253a0, but, notably,
this alternate procedure isn't followed for regular table renames, but
only for
[https://github.com/django/django/blob/3.2.13/django/db/backends/sqlite3/schema.py#L142-L157
""non-rename or column addition operations""]. So a standard `RenameModel`
still uses a standard `ALTER TABLE ... RENAME TO ...` which, if pragmas
are set as above, won't correctly repoint foreign keys.
I ''think'' that what all this might mean is that `RenameModel` foreign
key repointing was broken for a long time on versions of SQLite with
`foreign_keys=0` and `legacy_alter_table=1` (including versions 3.2 and
4.0). Commit 063cf98d3a6839f40c423cbd845def429c5cf0ce fixes that by always
setting `legacy_alter_table = OFF`, thus avoiding the bad configuration
even with simple `ALTER TABLE ... RENAME TO ...` statements.
Might it be possible to get this fix backported to currently supported
versions of Django?
(Another option might be to use the alternate procedure even for basic
table renames.)
--
Ticket URL: <https://code.djangoproject.com/ticket/31765#comment:20>