Opened 6 weeks ago

Closed 3 weeks ago

#31765 closed Bug (fixed)

schema.tests.SchemaTests.test_db_table fails on MacOS

Reported by: Tom Forbes Owned by: Tom Forbes
Component: Uncategorized Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

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

Change History (13)

comment:1 Changed 6 weeks ago 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)')

comment:2 Changed 6 weeks ago by Tom Forbes

Has patch: set
Triage Stage: UnreviewedAccepted

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.

comment:3 Changed 6 weeks ago 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?

comment:4 Changed 5 weeks ago 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.

comment:5 Changed 5 weeks ago 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...) 🤔

comment:6 Changed 5 weeks ago 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.

comment:7 Changed 5 weeks ago by Claude Paroz

What about Simon's suggestion on the pull request?

comment:8 Changed 5 weeks ago 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.

comment:9 Changed 5 weeks ago 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 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.)

Last edited 5 weeks ago by Carlton Gibson (previous) (diff)

comment:10 in reply to:  9 Changed 5 weeks ago by 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.

comment:11 Changed 5 weeks ago by Tom Forbes

Owner: changed from nobody to Tom Forbes
Status: newassigned

comment:12 Changed 3 weeks ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:13 Changed 3 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 80a8be0:

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

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