Loading
Commits on Source 46
-
Josh Bleecher Snyder authored
The column values and rowid were swapped; vtabUpdateTrampoline wasn't following SQLite's xUpdate contract. The existing tests didn't catch this because they used only a single column. Add more columns, and expand the test.
-
Josh Bleecher Snyder authored
This is pretty standard Go fare, but it's worth calling out as a cheap one-liner. The alternative is to remove this restriction, but I don't think it's worth the mutex overhead.
-
-
iKey2 is an int64, not two int32s. The trampoline's parameter list appears to have been modeled after the internal SQLite function sqlite3VdbePreUpdateHook, which takes (i64 iKey1, int iReg, int iBlobWrite). But that internal function computes iKey2 from iReg before invoking the public callback, whose actual signature ends with (sqlite3_int64 iKey1, sqlite3_int64 iKey2). The trampoline declared the final slot as (iReg int32, iBlobWrite int32) instead of (iKey2 int64), which works except for large rowids. Depending on the arch, this likely manifests as truncation. It's rather unclear how important this is, but on the other hand, the fix is small and targeted and highly testable, and for e.g. snowflake-style IDs, it'd show up as silent corruption.
-
-
cznic authored
-
-
Josh Bleecher Snyder authored
-
Josh Bleecher Snyder authored
tls.Alloc is inappropriate here, because sqlite owns this pointer and may choose to resize it; we must use a sqlite allocator. The test triggers a crash using a close, but it is also possible (albeit longer and noisier) to trigger via a re-alloc/re-size.
-
-
cznic authored
-
Josh Bleecher Snyder authored
How to handle Deserialize(nil) is a judgment call. sqlite3_deserialize(db, schema, NULL, 0, 0, ...) disconnects and starts fresh. But in database/sql world, connections are pooled, so that seems bad. But nil as an input is guaranteed to fail, possibly in ugly ways, if not handled explicitly. Given that we're in Go, and creating a new DB is trivial, opt for safety and explicitness and early feedback, and declare Deserialize(nil) to be an error and handle it explicitly.
-
-
cznic authored
-
Similar to 5e394aa2, except without practical consequences, and therefore untestable. Low priority, but trivial. Fixing only to avoid leaving a code rake.
-
-
cznic authored
-
`go test -race .` fails for me without this fix. The sqlite3_module struct was allocated on the Go heap and passed to sqlite3_create_module_v2 as a uintptr. Later when we access fields through unsafe.Pointer arithmetic, Go's checkptr instrumentation rejects the dereference because the pointer was round-tripped through a uintptr in C-allocated memory. Use libc.Xcalloc instead, matching the pattern in mutex.go and the VFS code. The allocation is intentionally never freed; modules are registered once and must outlive every connection that references them. There is no UnregisterModule API. This is a prerequisite change to another fix in the vtab code which requires `go test -race` to reproduce.
-
-
cznic authored
-
The non-recursive branch of mutexTry wrote mutex.id with a plain store while mutexHeld/mutexNotheld read it with atomic.LoadInt32. Mixing atomic and non-atomic accesses to the same word is a data race under Go's memory model. Use atomic.StoreInt32, consistent with every other write site (mutexEnter, mutexLeave, and the recursive branch of mutexTry). I have a regression test for this, but I'm not convinced it is worth adding. It's a lot of code and compute for an obvious fix that is unlikely to regress.
-
-
cznic authored
-
-
-
cznic authored
-
The exec path treated SQLITE_ROW identically to SQLITE_DONE, calling newResult after a single step. For DML with RETURNING clauses that affect multiple rows, this meant the statement was never stepped to completion, causing sqlite3_changes() to return stale counts. Continue stepping until SQLITE_DONE before reading the result. This could manifest as a performance regression: we're doing strictly more work by draining all the rows instead of abandoning them. But as Russ Cox says, I can make anything fast if it doesn't need to be correct. This brings this Exec implementation in line with C sqlite3_exec in several ways: - Row draining: C sqlite3_exec loops sqlite3_step until rc != SQLITE_ROW, then finalizes. - Result after completion: C sqlite3_exec does finalize-after-loop. - Unconditional drain: C sqlite3_exec with a NULL callback still steps through every row. - Partial commit on interruption: C returns SQLITE_ABORT if the callback returns non-zero mid-drain, but already-stepped rows stay committed. (Use an explicit transaction for atomicity.) - Multi-statement iteration: C's outer loop prepares and fully steps each sub-statement.
-
-
cznic authored
-
-
cznic authored
-
Josh Bleecher Snyder authored
registerSingleModule set FiVersion = 1, but also populated FxSavepoint, FxRelease, and FxRollbackTo. SQLite gates dispatch of those callbacks on iVersion >= 2, so they were silently ignored. Begin/Sync/Commit/Rollback (version 1 methods) worked fine, which made the gap easy to miss. iVersion was 1 from the initial commit. Version 2 does not change any version-1 callback behavior. The only behavioral change is that SQLite now dispatches the three savepoint callbacks, and it still checks each function pointer is non-zero before calling. The trampolines already handle modules that don't implement the methods (type-assert, return SQLITE_OK). So the bump to version 2 is safe. Version 3 adds xShadowName. Version 4 adds xIntegrity. Neither callback is set, so those remain inert; there's no need to upgrade past 2. Extend the existing updater vtab test to implement Transactional and verify that savepoint/release/rollbackto are dispatched.
-
-
cznic authored
-
The current code assumes a well-behavior io.Reader, which works until it doesn't. Add protection: convert f.Read(b) to io.ReadFull(f, b). While we're here, use the new clear built-in. Add test coverage, including tests that would have caught the initial bugs and tests ensuring tail clearing.
-
-
cznic authored
-
Two upcoming commits need a bit more control over exactly which db is used to generate an error. In anticipation of that, do some light refactoring. While we're here handle db == 0 gracefully: skip sqlite3_errmsg, which would misleadingly return "out of memory" for a NULL handle.
-
-
cznic authored
-
Josh Bleecher Snyder authored
Per SQLite docs, sqlite3_backup_init stores errors on the destination database handle. In backup mode the destination is remoteConn.db; in restore mode it is c.db. Previously errcode was always read from remoteConn.db (wrong for restore) and errmsg was always read from c.db via c.errstr (wrong for backup). Read both from the actual destination handle.
-
-
cznic authored
-
When sqlite3_open_v2 fails, the SQLite docs require the caller to close the handle it may have allocated. The previous code did not, leaking the sqlite3 struct and potentially VFS-level state. Additionally, newConn did not close libc.TLS on the openV2 failure path, leaking C-heap memory. Lastly, the error message was misleading: errstr called sqlite3_errmsg with c.db == 0 (not yet set), which always returns "out of memory", producing errors like "unable to open database file: out of memory (14)" regardless of the actual cause. Fix all of those.
-
-
cznic authored