schema.tests.SchemaTests.test_db_table fails on MacOS

[Tom Forbes]

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')
?          +++++

----------------------------------------------------------------------

[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)')

[Tom Forbes]

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.

[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?

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

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

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

[Claude Paroz]

What about Simon's suggestion on the pull request?

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

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

[Tom Forbes]

Replying to Carlton Gibson:

It's the sort of thing I might say to add a note to ​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.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 80a8be0:

Fixed #31765 -- Disabled bundled SQLite renaming atomic references on macOS 10.15.

[Skyler Cain]

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

[Carlton Gibson]

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

[Carlton Gibson]

OK, I found it. It's to do with the legacy_alter_table config option. (​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.

[GitHub <noreply@…>]

In 063cf98:

Fixed #31765 -- Enforced enhanced ALTER TABLE behavior for SQLite connections.

[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 ​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 ​here. This was implemented in 7289874adceec46b5367ec3157cdd10c711253a0, but, notably, this alternate procedure isn't followed for regular table renames, but only for ​"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.)