diff --git a/docs/runtime/sqlite.mdx b/docs/runtime/sqlite.mdx index bed3c16f5917..1ed804de93df 100644 --- a/docs/runtime/sqlite.mdx +++ b/docs/runtime/sqlite.mdx @@ -350,7 +350,7 @@ query.run(); Internally, this calls [`sqlite3_reset`](https://www.sqlite.org/capi3ref.html#sqlite3_reset) and calls [`sqlite3_step`](https://www.sqlite.org/capi3ref.html#sqlite3_step) once. Stepping through all the rows is not necessary when you don't care about the results. -The `lastInsertRowid` property is the ID of the last row inserted into the database. The `changes` property is the number of rows affected by the query. +The `lastInsertRowid` property is the ID of the last row inserted into the database. The `changes` property is the number of rows that the query inserts, updates, or deletes itself. Rows that a trigger or a foreign key action such as `ON DELETE CASCADE` changes do not count. A schema statement such as `CREATE VIRTUAL TABLE` can report rows that a virtual table module, for example FTS5, writes to its own tables. ### `.as(Class)` - Map query results to a class diff --git a/packages/bun-types/sqlite.d.ts b/packages/bun-types/sqlite.d.ts index 10e134ec4c45..3adfa1e5ff8a 100644 --- a/packages/bun-types/sqlite.d.ts +++ b/packages/bun-types/sqlite.d.ts @@ -1309,7 +1309,13 @@ declare module "bun:sqlite" { */ export interface Changes { /** - * The number of rows changed by the last `run` or `exec` call. + * The number of rows that the last `run` or `exec` call inserted, updated, or deleted itself. + * + * Rows that a trigger or a foreign key action such as `ON DELETE CASCADE` changes do not count. + * {@link Database.run} with several statements in one string reports the sum of the statements. + * + * One limit: a schema statement such as `CREATE VIRTUAL TABLE` can report rows that a virtual + * table module (for example FTS5) writes to its own tables while the statement runs. */ changes: number; diff --git a/src/jsc/bindings/sqlite/JSSQLStatement.cpp b/src/jsc/bindings/sqlite/JSSQLStatement.cpp index 7c3c011e2ffd..0d9b838aa79e 100644 --- a/src/jsc/bindings/sqlite/JSSQLStatement.cpp +++ b/src/jsc/bindings/sqlite/JSSQLStatement.cpp @@ -550,6 +550,8 @@ class JSSQLStatement : public JSC::JSDestructibleObject { // Created by db.query(); close(false) finalizes these but leaves db.prepare() statements usable. bool ownedByDatabase : 1 = false; bool finalizedByClose : 1 = false; + // An INSERT, UPDATE or DELETE: every run sets sqlite3_changes64(), so run() reads no other counter. + bool countsChanges : 1 = false; protected: JSSQLStatement(JSC::Structure* structure, JSDOMGlobalObject& globalObject, sqlite3_stmt* stmt, VersionSqlite3* version_db, int64_t memorySizeChange = 0) @@ -1469,6 +1471,12 @@ static bool isSkippedInSQLiteQuery(const char c) return c == ' ' || c == ';' || (c >= '\t' && c <= '\r'); } +// sqlite3_changes64() persists across statements, so only a statement that moved the total reports it (#43306). +static sqlite3_int64 directChangesSince(sqlite3* db, sqlite3_int64 totalChangesBefore) +{ + return sqlite3_total_changes64(db) == totalChangesBefore ? 0 : sqlite3_changes64(db); +} + // This runs a query one-off // without the overhead of a long-lived statement object // does not return anything @@ -1541,7 +1549,7 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteFunction, (JSC::JSGlobalObject * l bool strict = internalFlagsValue.isInt32() && (internalFlagsValue.asInt32() & kStrictFlag) != 0; bool safeIntegers = internalFlagsValue.isInt32() && (internalFlagsValue.asInt32() & kSafeIntegersFlag) != 0; - const int total_changes_before = sqlite3_total_changes(db); + sqlite3_int64 changes = 0; while (sqlStringHead && sqlStringHead < end) { if (isSkippedInSQLiteQuery(*sqlStringHead)) [[unlikely]] { @@ -1599,10 +1607,17 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteFunction, (JSC::JSGlobalObject * l didSetBindings = true; } + // COMMIT, SAVEPOINT and RELEASE are read-only but can flush FTS5, which moves sqlite3_total_changes64(). + const bool canChangeRows = !sqlite3_stmt_readonly(sql.stmt); + const sqlite3_int64 total_changes_before = canChangeRows ? sqlite3_total_changes64(db) : 0; + do { rc = sqlite3_step(sql.stmt); } while (rc == SQLITE_ROW); + if (canChangeRows) + changes += directChangesSince(db, total_changes_before); + didExecuteAny = true; sqlStringHead = tail; } @@ -1618,9 +1633,8 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteFunction, (JSC::JSGlobalObject * l } if (auto* diff = dynamicDowncast(diffValue)) { - const int total_changes_after = sqlite3_total_changes(db); int64_t last_insert_rowid = sqlite3_last_insert_rowid(db); - diff->putInternalField(vm, 0, JSC::jsNumber(total_changes_after - total_changes_before)); + diff->putInternalField(vm, 0, JSC::jsNumber(changes)); if (safeIntegers) { auto* bigInt = JSBigInt::createFrom(lexicalGlobalObject, last_insert_rowid); RETURN_IF_EXCEPTION(scope, {}); @@ -2630,10 +2644,13 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteStatementFunctionRun, (JSC::JSGlob } auto* db = sqlite3_db_handle(stmt); - int total_changes_before = sqlite3_total_changes(db); + const bool canChangeRows = !sqlite3_stmt_readonly(stmt); + // Each counter read locks the connection mutex, so only a statement of unknown kind reads the total. + const bool checkTotal = canChangeRows && !castedThis->countsChanges; + const sqlite3_int64 total_changes_before = checkTotal ? sqlite3_total_changes64(db) : 0; int status = sqlite3_step(stmt); - if (!sqlite3_stmt_readonly(stmt)) { + if (canChangeRows) { castedThis->version_db->version++; } @@ -2656,9 +2673,16 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteStatementFunctionRun, (JSC::JSGlob } if (auto* diff = dynamicDowncast(diffValue)) { - const int total_changes_after = sqlite3_total_changes(db); int64_t last_insert_rowid = sqlite3_last_insert_rowid(db); - diff->putInternalField(vm, 0, JSC::jsNumber(total_changes_after - total_changes_before)); + sqlite3_int64 changes = 0; + if (castedThis->countsChanges) { + changes = sqlite3_changes64(db); + } else if (checkTotal) { + changes = directChangesSince(db, total_changes_before); + // SQLite rejects bound parameters in DDL and PRAGMA, so a statement that has them and changed rows is DML. + castedThis->countsChanges = changes != 0 && sqlite3_bind_parameter_count(stmt) > 0; + } + diff->putInternalField(vm, 0, JSC::jsNumber(changes)); if (castedThis->useBigInt64) { JSValue lastRowIdBigInt = JSBigInt::createFrom(lexicalGlobalObject, last_insert_rowid); RETURN_IF_EXCEPTION(scope, {}); diff --git a/src/jsc/bindings/sqlite/lazy_sqlite3.h b/src/jsc/bindings/sqlite/lazy_sqlite3.h index 818727dd0da7..e59556cb3642 100644 --- a/src/jsc/bindings/sqlite/lazy_sqlite3.h +++ b/src/jsc/bindings/sqlite/lazy_sqlite3.h @@ -59,6 +59,7 @@ typedef int (*lazy_sqlite3_finalize_type)(sqlite3_stmt* pStmt); typedef void (*lazy_sqlite3_free_type)(void*); typedef int (*lazy_sqlite3_get_autocommit_type)(sqlite3*); typedef int (*lazy_sqlite3_total_changes_type)(sqlite3*); +typedef sqlite3_int64 (*lazy_sqlite3_total_changes64_type)(sqlite3*); typedef int (*lazy_sqlite3_config_type)(int, ...); typedef int (*lazy_sqlite3_open_v2_type)(const char* filename, sqlite3** ppDb, int flags, const char* zVfs); typedef int (*lazy_sqlite3_prepare_v2_type)(sqlite3* db, const char* zSql, int nByte, sqlite3_stmt** ppStmt, const char** pzTail); @@ -185,6 +186,7 @@ inline lazy_sqlite3_extended_errcode_type lazy_sqlite3_extended_errcode; inline lazy_sqlite3_error_offset_type lazy_sqlite3_error_offset; inline lazy_sqlite3_bind_parameter_name_type lazy_sqlite3_bind_parameter_name; inline lazy_sqlite3_total_changes_type lazy_sqlite3_total_changes; +inline lazy_sqlite3_total_changes64_type lazy_sqlite3_total_changes64; inline lazy_sqlite3_last_insert_rowid_type lazy_sqlite3_last_insert_rowid; inline lazy_sqlite3_exec_type lazy_sqlite3_exec; inline lazy_sqlite3_limit_type lazy_sqlite3_limit; @@ -281,6 +283,7 @@ inline lazy_sqlite3changeset_apply_type lazy_sqlite3changeset_apply; #define sqlite3_error_offset lazy_sqlite3_error_offset #define sqlite3_bind_parameter_name lazy_sqlite3_bind_parameter_name #define sqlite3_total_changes lazy_sqlite3_total_changes +#define sqlite3_total_changes64 lazy_sqlite3_total_changes64 #define sqlite3_last_insert_rowid lazy_sqlite3_last_insert_rowid #define sqlite3_exec lazy_sqlite3_exec #define sqlite3_limit lazy_sqlite3_limit @@ -433,6 +436,7 @@ inline int lazyLoadSQLiteUnlocked(WTF::String* errorMessage = nullptr) lazy_sqlite3_error_offset = (lazy_sqlite3_error_offset_type)dlsym(sqlite3_handle, "sqlite3_error_offset"); lazy_sqlite3_bind_parameter_name = (lazy_sqlite3_bind_parameter_name_type)dlsym(sqlite3_handle, "sqlite3_bind_parameter_name"); lazy_sqlite3_total_changes = (lazy_sqlite3_total_changes_type)dlsym(sqlite3_handle, "sqlite3_total_changes"); + lazy_sqlite3_total_changes64 = (lazy_sqlite3_total_changes64_type)dlsym(sqlite3_handle, "sqlite3_total_changes64"); lazy_sqlite3_last_insert_rowid = (lazy_sqlite3_last_insert_rowid_type)dlsym(sqlite3_handle, "sqlite3_last_insert_rowid"); lazy_sqlite3_exec = (lazy_sqlite3_exec_type)dlsym(sqlite3_handle, "sqlite3_exec"); lazy_sqlite3_limit = (lazy_sqlite3_limit_type)dlsym(sqlite3_handle, "sqlite3_limit"); @@ -506,6 +510,12 @@ inline int lazyLoadSQLiteUnlocked(WTF::String* errorMessage = nullptr) return static_cast(lazy_sqlite3_changes(db)); }; } + // sqlite3_total_changes64 is also 3.37.0+. + if (!lazy_sqlite3_total_changes64) { + lazy_sqlite3_total_changes64 = [](sqlite3* db) -> sqlite3_int64 { + return static_cast(lazy_sqlite3_total_changes(db)); + }; + } return 0; } diff --git a/test/js/bun/sqlite/sqlite.test.js b/test/js/bun/sqlite/sqlite.test.js index 81297e59b5fd..06662cadfbf4 100644 --- a/test/js/bun/sqlite/sqlite.test.js +++ b/test/js/bun/sqlite/sqlite.test.js @@ -2146,6 +2146,196 @@ it("reports changes in Statement#run", () => { expect(db.query(sql).run().changes).toBe(2); }); +describe("changes counts only the rows that the statement changes itself", () => { + const entryPoints = { + "Database#run": (db, sql, ...params) => db.run(sql, ...params), + "Database#exec": (db, sql, ...params) => db.exec(sql, ...params), + "Statement#run from prepare()": (db, sql, ...params) => { + using stmt = db.prepare(sql); + return stmt.run(...params); + }, + "Statement#run from query()": (db, sql, ...params) => db.query(sql).run(...params), + }; + + describe.each(Object.entries(entryPoints))("%s", (_, run) => { + it("not the shadow tables of a virtual table", () => { + using db = new Database(":memory:"); + db.exec("CREATE VIRTUAL TABLE ft USING fts5(a, b); CREATE VIRTUAL TABLE rt USING rtree(id, x0, x1);"); + + expect({ + fts5Insert: run(db, "INSERT INTO ft VALUES ('alpha beta', 'gamma')").changes, + fts5InsertTwoRows: run(db, "INSERT INTO ft VALUES (?, ?), (?, ?)", "a", "b", "c", "d").changes, + fts5UpdateTwoRows: run(db, "UPDATE ft SET b = 'delta' WHERE rowid <= 2").changes, + fts5DeleteThreeRows: run(db, "DELETE FROM ft").changes, + rtreeInsert: run(db, "INSERT INTO rt VALUES (1, 0, 1)").changes, + rtreeDelete: run(db, "DELETE FROM rt WHERE id = 1").changes, + }).toEqual({ + fts5Insert: 1, + fts5InsertTwoRows: 2, + fts5UpdateTwoRows: 2, + fts5DeleteThreeRows: 3, + rtreeInsert: 1, + rtreeDelete: 1, + }); + }); + + it("not the rows that a trigger writes", () => { + using db = new Database(":memory:"); + db.exec(` + CREATE TABLE t (a); + CREATE TABLE log (x); + CREATE TRIGGER tr AFTER INSERT ON t BEGIN + INSERT INTO log VALUES (new.a); + INSERT INTO log VALUES (-new.a); + END; + `); + + expect(run(db, "INSERT INTO t VALUES (1)").changes).toBe(1); + expect(run(db, "INSERT INTO t VALUES (?), (?)", 2, 3).changes).toBe(2); + expect(db.query("SELECT x FROM log ORDER BY rowid").values()).toEqual([[1], [-1], [2], [-2], [3], [-3]]); + }); + + it("not the rows that a foreign key action deletes", () => { + using db = new Database(":memory:"); + db.exec(` + PRAGMA foreign_keys = ON; + CREATE TABLE parent (id INTEGER PRIMARY KEY); + CREATE TABLE child (id INTEGER PRIMARY KEY, parent_id REFERENCES parent (id) ON DELETE CASCADE); + INSERT INTO parent VALUES (1), (2); + INSERT INTO child VALUES (10, 1), (11, 1), (12, 1), (20, 2); + `); + + expect(run(db, "DELETE FROM parent WHERE id = 1").changes).toBe(1); + expect(db.query("SELECT id FROM child").values()).toEqual([[20]]); + }); + + // https://www.sqlite.org/c3ref/changes.html: an INSTEAD OF trigger makes the change, so the statement changes no row. + it("not the rows that an INSTEAD OF trigger writes for a view", () => { + using db = new Database(":memory:"); + db.exec(` + CREATE TABLE t (a); + CREATE VIEW v AS SELECT a FROM t; + CREATE TRIGGER vi INSTEAD OF INSERT ON v BEGIN + INSERT INTO t VALUES (new.a); + END; + `); + + expect(run(db, "INSERT INTO v VALUES (1)").changes).toBe(0); + expect(db.query("SELECT a FROM t").values()).toEqual([[1]]); + }); + + it("0 for a statement that changes no row, not the count of the write before it", () => { + using db = new Database(":memory:"); + db.exec("CREATE TABLE t (a)"); + + // Each statement runs right after a write of two rows, the count that sqlite3_changes() still holds. + const afterWrite = sql => { + expect(run(db, "INSERT INTO t VALUES (1), (2)").changes).toBe(2); + return run(db, sql).changes; + }; + expect({ + select: afterWrite("SELECT a FROM t"), + createTable: afterWrite("CREATE TABLE other (a)"), + begin: afterWrite("BEGIN"), + commit: afterWrite("COMMIT"), + updateNoRow: afterWrite("UPDATE t SET a = 0 WHERE 0"), + }).toEqual({ select: 0, createTable: 0, begin: 0, commit: 0, updateNoRow: 0 }); + }); + + // FTS5 keeps the rows of a transaction in memory. SAVEPOINT, RELEASE and COMMIT make it write them to its shadow tables. + it("0 for a SAVEPOINT, RELEASE or COMMIT that makes FTS5 write its pending rows", () => { + using db = new Database(":memory:"); + db.exec("CREATE VIRTUAL TABLE ft USING fts5(a, b)"); + const insert = () => expect(run(db, "INSERT INTO ft VALUES ('alpha beta', 'gamma')").changes).toBe(1); + + run(db, "BEGIN"); + insert(); + const savepoint = run(db, "SAVEPOINT s1").changes; + insert(); + run(db, "SAVEPOINT s2"); + insert(); + const release = run(db, "RELEASE s1").changes; + insert(); + const commit = run(db, "COMMIT").changes; + expect({ savepoint, release, commit }).toEqual({ savepoint: 0, release: 0, commit: 0 }); + }); + }); + + it.each(["run", "exec"])("Database#%s does not add the FTS5 writes of a COMMIT in the same string", method => { + using db = new Database(":memory:"); + db.exec("CREATE VIRTUAL TABLE ft USING fts5(a, b)"); + + const inserts = "INSERT INTO ft VALUES ('a', 'b'); INSERT INTO ft VALUES ('c', 'd'), ('e', 'f');"; + expect({ + autocommit: db[method](inserts).changes, + transaction: db[method](`BEGIN; ${inserts} COMMIT;`).changes, + }).toEqual({ autocommit: 3, transaction: 3 }); + }); + + it.each(["run", "exec"])("Database#%s adds up the statements of a multi-statement string", method => { + using db = new Database(":memory:"); + db.exec(` + CREATE TABLE t (a); + CREATE TABLE log (x); + CREATE TRIGGER tr AFTER INSERT ON t BEGIN + INSERT INTO log VALUES (new.a); + END; + `); + + // The SELECT and the CREATE TABLE change no row, so they must not add the count of the INSERT before them again. + const sql = "INSERT INTO t VALUES (1); INSERT INTO t VALUES (2), (3); SELECT a FROM t; CREATE TABLE other (a);"; + expect(db[method](sql).changes).toBe(3); + expect(db[method]("SELECT 1; SELECT 2;").changes).toBe(0); + expect(db.query("SELECT x FROM log ORDER BY rowid").values()).toEqual([[1], [2], [3]]); + }); + + // With foreign keys on, DROP TABLE runs a DELETE that SQLite counts. The second DROP is a no-op and sets no counter. + it.each(["prepare", "query"])("a reused DROP TABLE Statement from %s() reports 0 when it is a no-op", method => { + using db = new Database(":memory:"); + db.exec(` + PRAGMA foreign_keys = ON; + CREATE TABLE parent (id INTEGER PRIMARY KEY); + CREATE TABLE child (id INTEGER PRIMARY KEY, parent_id REFERENCES parent (id) ON DELETE CASCADE); + CREATE TABLE t (a); + INSERT INTO parent VALUES (1), (2), (3); + INSERT INTO child VALUES (10, 1); + `); + using drop = db[method]("DROP TABLE IF EXISTS parent"); + + expect([ + drop.run().changes, + db.run("INSERT INTO t VALUES (1), (2), (3), (4), (5)").changes, + drop.run().changes, + ]).toEqual([3, 5, 0]); + }); + + // A Statement with bound parameters that changed rows once reads only sqlite3_changes64() on later runs. + it.each(["prepare", "query"])("a reused Statement from %s() reports its own rows on every run", method => { + using db = new Database(":memory:"); + db.exec(` + CREATE TABLE t (id INTEGER PRIMARY KEY, a); + CREATE TABLE log (x); + CREATE TRIGGER tr AFTER UPDATE ON t BEGIN + INSERT INTO log VALUES (new.id); + END; + `); + using insert = db[method]("INSERT INTO t (a) VALUES (?), (?), (?)"); + using update = db[method]("UPDATE t SET a = ? WHERE id <= ?"); + using select = db[method]("SELECT count(*) FROM t"); + + expect([ + update.run("x", 0).changes, + insert.run(1, 2, 3).changes, + update.run("x", 2).changes, + select.run().changes, + insert.run(4, 5, 6).changes, + update.run("y", 0).changes, + select.run().changes, + update.run("y", 1).changes, + ]).toEqual([0, 3, 2, 0, 3, 0, 0, 1]); + }); +}); + it("#13082", async () => { async function run(op) { const stmt = (() => { diff --git a/test/js/sql/sqlite-sql.test.ts b/test/js/sql/sqlite-sql.test.ts index ed0567c186fb..02e36271ac72 100644 --- a/test/js/sql/sqlite-sql.test.ts +++ b/test/js/sql/sqlite-sql.test.ts @@ -2727,6 +2727,27 @@ describe("Triggers and Views", () => { const alerts2 = await sql`SELECT * FROM reorder_alerts`; expect(alerts2).toHaveLength(1); }); + + test("count is the rows that the statement changes itself, not the rows that a trigger or FTS5 writes", async () => { + await sql`CREATE TABLE t (a)`; + await sql`CREATE TABLE log (x)`; + await sql`CREATE TRIGGER tr AFTER INSERT ON t BEGIN + INSERT INTO log VALUES (new.a); + INSERT INTO log VALUES (-new.a); + END`; + await sql`CREATE VIRTUAL TABLE ft USING fts5(a, b)`; + + const inserted = await sql`INSERT INTO t VALUES (1), (2)`; + const indexed = await sql`INSERT INTO ft VALUES ('alpha beta', 'gamma')`; + // The COMMIT makes FTS5 write its pending rows. Those writes are not rows of the COMMIT. + const inTransaction = await sql.unsafe("BEGIN; INSERT INTO ft VALUES ('delta', 'epsilon'); COMMIT;"); + expect({ inserted: inserted.count, indexed: indexed.count, inTransaction: inTransaction.count }).toEqual({ + inserted: 2, + indexed: 1, + inTransaction: 1, + }); + expect(await sql`SELECT x FROM log ORDER BY rowid`.values()).toEqual([[1], [-1], [2], [-2]]); + }); }); describe("Indexes and Query Optimization", () => {