Check database file encoding to set appropriate opening mode #3792
Conversation
|
@mgrojo I am requesting a review since you discussed the issue together in the related thread. |
| const char* encodingBytes = header.constData() + 56; | ||
| // The variable 'encoding' will have one of the following values: | ||
| // 1: UTF-8, 2: UTF-16LE, 3: UTF-16BE | ||
| quint32 encoding = ((quint8)encodingBytes[0] << 24) | |
There was a problem hiding this comment.
From https://www.sqlite.org/fileformat.html#text_encoding
. The sqlite3.h header file defines C-preprocessor macros SQLITE_UTF8 as 1, SQLITE_UTF16LE as 2, and SQLITE_UTF16BE as 3, to use in place of the numeric codes for the text encoding.
Could you use that instead of these magic numbers? You could also reference that page.
On the other hand, there's no reference to any preprocessor definition for 56, nor to the database header size. Is this the recommended way to determine the encoding? What is the source documentation you have found for this?
There was a problem hiding this comment.
Good idea, changed via afb2c43.
Regarding the 56 offset, you can refer to the file header specifications provided by SQLite.
| { | ||
| status = sqlite3_open16(db.constData(), &_db); | ||
| if (status == SQLITE_OK && readOnly) | ||
| sqlite3_exec(_db, "PRAGMA query_only = TRUE;", nullptr, nullptr, nullptr); |
There was a problem hiding this comment.
According to https://www.sqlite.org/pragma.html#pragma_query_only
However, the database is not truly read-only. You can still run a checkpoint or a COMMIT and the return value of the sqlite3_db_readonly() routine is not affected.
Is strange that this is the solution for those two issues (#3714 and #3636). Are we working around some bug in SQLite3? Shouldn't it detect the database encoding and act accordingly?
The documentation for these functions say that they determine the default encoding for databases created using these functions, not when opening already created databases. Everything seems to indicate that SQLite3 should do that automatically.
There's nothing written, but my impression is that sqlite3_open16 is deprecated in favour of sqlite3_open_v2, and you only have to convert to UTF-8 the filename if running on an OS with UTF-16 filenames (I think Windows is like that, but not Linux; I don't know about macOS). Then, if creating the DB, you use "pragma ENCODING" to set the database encoding.
There was a problem hiding this comment.
The sqlite3_open16 function doesn’t provide an option to open the file in read-only mode, unlike sqlite3_open_v2, so that’s why it was done this way.
It might be possible to use a URI filename as shown like this: file:data.db?mode=ro
but to do so, we would need to compile and provide the SQLite binaries ourselves for Linux, macOS, and Windows.
|
I think we are affected by this bug (we are even mentioned): https://sqlite.org/forum/forumpost/09503b4d33 That explains my build does not show the problem, it is based on "SQLCipher Version 3.4.1 (based on SQLite 3.15.2)." I don't know how to check if we have the fix in our version of SQLite, or even if their fix is released already or not. @justinclift do you know how to check that? |
|
That SQLite bug also explains why it is reproduced with AppImage for v3.13.1 (based on SQLite 3.46.1) but not for v3.13.0 (based on SQLite 3.15.2). If we look only at the dates, I guess the fix is in SQLite 3.47.0 released on 2024-10-21. |
|
It still happens in https://github.com/sqlitebrowser/sqlitebrowser/releases/download/continuous/DB.Browser.for.SQLite-dev-5ff8793-x86.64.AppImage which is based on 3.47.0. I think the anonymous user is right, and they haven't been carefully listened to. The problem is still there after the supposed fix. @lucydodo I think your PR shouldn't be merged. The encoding detection should be automatically done by SQLite3 and you have demonstrated with your changes that it isn't correctly doing it, and that causes the DB lock. SQLite3 is the one that should fix it, since this worked with previous versions. |
|
I added a comment on the forum to explain what we are seeing, which is "Awaiting Moderator Approval". |
Nah. @lucydodo has been creating the SQLite build pieces on our various platforms (except Snap) for a while now. It's probably just going to be a case of checking if it's an affected version of SQLite or not. |
|
Hopefully the people on the SQLite Forum have a idea of how to assist in relation to @mgrojo's post. |
@mgrojo There is absolutely no issue, and I agree with what you've said.
Yes, I have been in charge of the overall deployment process across each platfrom recently.
But in today's nightly build, which uses SQLite version 3.47.0, the issue is still occurring. |
|
|
|
Is there a ~recent version of SQLite that doesn't have this problem? If there is, then maybe we'll need to revert our builds to using that until the next SQLite release? |
|
The version that includes this bug fix has not been released yet. Also I'm not entirely sure which version to roll back to. |
|
A temporary build including the version of SQLite with this bug fixed has been released: https://github.com/sqlitebrowser/sqlitebrowser/releases/tag/issue%2F3714 |
This PR adds support for UTF-16LE and UTF-16BE database files. Related: #3714
Tested with the following three database files on macOS 15.0.1 (24A348) environment.
Any comments and review are welcome. Thanks :)