Repository navigation
bun:sqlite: count byteOffset from the start of the SQL text in run and exec
#45229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
🟣 pre-existing, not blocking: Callers of db.run()/db.exec() can receive a byteOffset that points past the statement that actually failed. At src/jsc/bindings/sqlite/JSSQLStatement.cpp:385-387 the helper adds byteOffsetBase to whatever sqlite3_error_offset returns, but SQLite leaves a stale non-negative offset in place when a prepare fails outside the parser: sqlite3ErrorWithMsg at sqlite3.c:148663 ("database schema is locked") and sqlite3.c:148681 ("statement too long") never reset errByteOffset. The base branch reported the stale value k; the PR reports k plus the start of a later statement. Fix: only add the base when the error came from the parser of this prepare, e.g. clear or snapshot the offset before sqlite3_prepare_v3 at JSSQLStatement.cpp:1567 and add the base only when it changed.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A prior statement on the same connection leaves errByteOffset >= 0 (any parse error, or a step-time re-prepare failure in statement N of the same run()). The next prepare in the loop at src/jsc/bindings/sqlite/JSSQLStatement.cpp:1567 fails outside the parser: sqlite3Prepare's "database schema is locked" path (sqlite3.c:148660-148665, shared-cache) or the SQLITE_TOOBIG path (sqlite3.c:148680-148683) go through sqlite3ErrorWithMsg, which sets errCode and pErr but never touches errByteOffset; sqlite3ErrorClear (sqlite3.c:36534-36539) runs only on success at sqlite3.c:148728. createSQLiteError at JSSQLStatement.cpp:385-387 sees byteOffset >= 0 and adds sqlStringHead - sqlStringStart. On the base branch the user got the stale k unchanged; now they get k + base, an offset the new JSDoc at packages/bun-types/sqlite.d.ts:1296-1301 tells them is the token position in the whole string. The dismissal argued "no worse" without reading these two non-resetting paths. Remedy: add the base only when the offset was produced by this prepare call.
Verification: sqlite3ErrorWithMsg (36582-36596) sets errCode and pErr and never touches errByteOffset. New code at src/jsc/bindings/sqlite/JSSQLStatement.cpp:385-387 adds the loop base from line 1570 to that stale value. In the realistic trigger the failing prepare is the FIRST statement of the run() (base = 0), and the PR reports the identical stale value the base branch reports.
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.
This is real, and it exists without this PR. I measured it on 1.4.3: after
db.run("SELECT 1 nope nope")(offset 14), a text longer thanSQLITE_LIMIT_SQL_LENGTHthrowsSQLITE_TOOBIG"statement too long" withbyteOffset14 throughrun,execandquery. ASQLITE_BUSYduring the schema load gives -1, so the two paths you name are the ones that keep the old value.This PR does not cause it and does not fix it. It changes the stale number only when that failure happens behind blanks or behind an earlier statement: 14 becomes 14 plus the base.
I did not take the snapshot before
sqlite3_prepare_v3. It adds asqlite3_error_offsetcall, with its mutex, to every statement of every successfulrun. "Changed" also cannot tell a new offset from an old one with the same value. The fix belongs increateSQLiteErrorfor all callers, and its test must not need 1 GB of text. I handed that off as separate work. The Notes of the PR body list it under "Not changed".