GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Check database file encoding to set appropriate opening mode by lucydodo · Pull Request #3792 · sqlitebrowser/sqlitebrowser

Conversation

@lucydodo

@lucydodo lucydodo commented Nov 2, 2024

Copy link
Copy Markdown
Member

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.

  • Chinook Database v1.4.5
  • LaurentGrenet's Database
  • Tafaza's Database

Any comments and review are welcome. Thanks :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lucydodo
Check database file encoding to set appropriate opening mode
25bac8d 
Fix: #3714
@lucydodo lucydodo added the bug Confirmed bugs or reports that are very likely to be bugs. label Nov 2, 2024
@lucydodo lucydodo added this to the 3.13.2 milestone Nov 2, 2024
@lucydodo
lucydodo requested a review from mgrojo November 2, 2024 14:26
@lucydodo lucydodo self-assigned this Nov 2, 2024
@lucydodo

lucydodo commented Nov 2, 2024

Copy link
Copy Markdown
Member Author

@mgrojo I am requesting a review since you discussed the issue together in the related thread.
If you have time, I would appreciate it if you could take a look. Thanks :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lucydodo lucydodo mentioned this pull request Nov 2, 2024
[Bug]: get database table is locked error when opening some dbs #3714
Closed
1 task done
mgrojo
mgrojo reviewed Nov 2, 2024
View reviewed changes

@mgrojo mgrojo left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this fixes the issue. Why did it work in the previous version?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/sqlitedb.cpp
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) |

@mgrojo mgrojo Nov 2, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lucydodo lucydodo Nov 2, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, changed via afb2c43.

https://www.sqlite.org/fileformat.html#database_header

Regarding the 56 offset, you can refer to the file header specifications provided by SQLite.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/sqlitedb.cpp
{
status = sqlite3_open16(db.constData(), &_db);
if (status == SQLITE_OK && readOnly)
sqlite3_exec(_db, "PRAGMA query_only = TRUE;", nullptr, nullptr, nullptr);

@mgrojo mgrojo Nov 2, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lucydodo lucydodo Nov 2, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lucydodo
Use the preprocessor macro instead of the magic number for the encoding
afb2c43 
value
@mgrojo

mgrojo commented Nov 2, 2024

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mgrojo

mgrojo commented Nov 2, 2024

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mgrojo

mgrojo commented Nov 2, 2024

Copy link
Copy Markdown
Member

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.
https://sqlite.org/forum/forumpost/568278e1d3768454

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mgrojo

mgrojo commented Nov 2, 2024

Copy link
Copy Markdown
Member

I added a comment on the forum to explain what we are seeing, which is "Awaiting Moderator Approval".
justinclift reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@justinclift

justinclift commented Nov 3, 2024

Copy link
Copy Markdown
Member

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?

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@justinclift

justinclift commented Nov 3, 2024

Copy link
Copy Markdown
Member

Hopefully the people on the SQLite Forum have a idea of how to assist in relation to @mgrojo's post.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lucydodo

lucydodo commented Nov 3, 2024

Copy link
Copy Markdown
Member Author

I think your PR shouldn't be merged.

@mgrojo There is absolutely no issue, and I agree with what you've said.
For now it's important to wait for SQLite's feedback, identify the exact cause of the issue, and then choose a better solution.

Nah. @lucydodo has been creating the SQLite build pieces on our various platforms (except Snap) for a while now.

Yes, I have been in charge of the overall deployment process across each platfrom recently.
Currently, the nightly builds for AppImage and Windows are set up to automatically fetch the latest versions of SQLCipher and SQLite. (I am also responsible for macOS, but it hasn't been updated to the latest version yet.)

If we look only at the dates, I guess the fix is in SQLite 3.47.0 released on 2024-10-21.

But in today's nightly build, which uses SQLite version 3.47.0, the issue is still occurring.
justinclift reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lucydodo
lucydodo marked this pull request as draft November 6, 2024 15:38
@lucydodo lucydodo added the invalid label Nov 6, 2024
@lucydodo lucydodo closed this Nov 6, 2024
@lucydodo
lucydodo deleted the issue/3714 branch November 6, 2024 16:12
@lucydodo

lucydodo commented Nov 6, 2024

Copy link
Copy Markdown
Member Author

{A9023B56-8580-45BB-8810-D4740C1C8E2A}
This bug has been identified as an issue on the SQLite side. I have confirmed that building the latest commit, 61cf538, from the SQLite master branch resolves the problem without needing to modify our app. I would like to express my gratitude to @mgrojo for helping me pinpoint the cause of the issue and to the SQLite team for fixing the bug.
mgrojo reacted with thumbs up emoji justinclift reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@justinclift

justinclift commented Nov 6, 2024

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lucydodo

lucydodo commented Nov 7, 2024

Copy link
Copy Markdown
Member Author

The version that includes this bug fix has not been released yet. Also I'm not entirely sure which version to roll back to.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lucydodo

lucydodo commented Nov 14, 2024

Copy link
Copy Markdown
Member Author

A temporary build including the version of SQLite with this bug fixed has been released: https://github.com/sqlitebrowser/sqlitebrowser/releases/tag/issue%2F3714
justinclift reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgrojo mgrojo mgrojo left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@lucydodo lucydodo

Labels

bug Confirmed bugs or reports that are very likely to be bugs. invalid

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@lucydodo @mgrojo @justinclift
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.