libsql-client-ts icon indicating copy to clipboard operation
libsql-client-ts copied to clipboard

Usage of transactions in `:memory:` database was broken with #105

Open epatey opened this issue 1 year ago • 10 comments

#105 regressed in memory databases when transactions are used. Once a transaction is created, the entire in memory database will be effectively discarded for subsequent db operations that the client makes.

From the regressing PR.

  1. A single connection is created lazily whenever user executes a statement
  2. When a transaction starts, the Transaction object takes ownership of the current transaction.
  3. Subsequent queries will create a new connection.

In particular, this line of code from within transaction, null's out this.#db.

Subsequent calls to #getDb will create a new database here. Because it's purely in memory, this essentially creates a new empty database.

I wonder if the fix would be as simple as skipping the dropping/recreating of the Database if path === ':memory'.

epatey avatar Jun 24 '24 14:06 epatey

async transaction(mode: TransactionMode = "write"): Promise<Transaction> {
    const db = this.#getDb();
    executeStmt(db, transactionModeToBegin(mode), this.#intMode);
    if (this.#path !== ':memory:') {
        this.#db = null; // A new connection will be lazily created on next use
    }
    return new Sqlite3Transaction(db, this.#intMode);
}

// Lazily creates the database connection and returns it
#getDb(): Database.Database {
    if (this.#db === null && this.$path !== ':memory:') {
        this.#db = new Database(this.#path, this.#options);
    }
    return this.#db;
}

epatey avatar Jun 24 '24 14:06 epatey

Related? https://github.com/tursodatabase/libsql/issues/1411

erkannt avatar Jun 28 '24 11:06 erkannt

This is fixed in 0.9.0 file::memory:?cache=shared

khuezy avatar Aug 15 '24 00:08 khuezy

This is fixed in 0.9.0

@khuezy, how does 0.9.0 fix this? Could you point out the relevant part in the diff?


file::memory:?cache=shared

For me, this revealed another problem: closing the database connection did not appear to be working. New connections were still using the old database, even after closing. Has anyone else encountered this too? @thewilkybarkid is this what you're referring to in the commit you've linked?


Based on @epatey's comment, I used the following patch to resolve:

diff --git a/node_modules/@libsql/client/lib-cjs/sqlite3.js b/node_modules/@libsql/client/lib-cjs/sqlite3.js
index f479612..5bb7f4d 100644
--- a/node_modules/@libsql/client/lib-cjs/sqlite3.js
+++ b/node_modules/@libsql/client/lib-cjs/sqlite3.js
@@ -145,7 +145,11 @@ class Sqlite3Client {
     async transaction(mode = "write") {
         const db = this.#getDb();
         executeStmt(db, (0, util_1.transactionModeToBegin)(mode), this.#intMode);
-        this.#db = null; // A new connection will be lazily created on next use
+
+        if (!this.#path.includes(":memory:")) {
+            this.#db = null; // A new connection will be lazily created on next use
+        }
+
         return new Sqlite3Transaction(db, this.#intMode);
     }
     async executeMultiple(sql) {
diff --git a/node_modules/@libsql/client/lib-esm/sqlite3.js b/node_modules/@libsql/client/lib-esm/sqlite3.js
index 8aa7047..22b6781 100644
--- a/node_modules/@libsql/client/lib-esm/sqlite3.js
+++ b/node_modules/@libsql/client/lib-esm/sqlite3.js
@@ -123,7 +123,11 @@ export class Sqlite3Client {
     async transaction(mode = "write") {
         const db = this.#getDb();
         executeStmt(db, transactionModeToBegin(mode), this.#intMode);
-        this.#db = null; // A new connection will be lazily created on next use
+
+        if (!this.#path.includes(":memory:")) {
+            this.#db = null; // A new connection will be lazily created on next use
+        }
+
         return new Sqlite3Transaction(db, this.#intMode);
     }
     async executeMultiple(sql) {

this.#path.includes(':memory:') handles the existence of the file scheme and potential query parameters:

  • :memory:
  • file::memory:
  • file::memory:?cache=shared

The modification in getDb did not seem necessary.

winghouchan avatar Dec 21 '24 03:12 winghouchan

@winghouchan Hi, my comment wasn't clear, the OP was in June so the diff was on an older version: https://github.com/tursodatabase/libsql-client-ts/compare/v0.7.0...v0.9.0

I'm not sure of the issue you're having, perhaps messaging the team on the discord will get their attention if there's an issue.

khuezy avatar Dec 23 '24 19:12 khuezy

@khuezy, so it wasn't 0.9.0 then?

Taking a look through PRs, are you referring to #220 (released in 0.8.0 (commit))?

winghouchan avatar Dec 23 '24 21:12 winghouchan

Ah, I see #220 intended to fix https://github.com/tursodatabase/libsql/issues/1411 which is a duplicate of this issue.

winghouchan avatar Dec 23 '24 21:12 winghouchan

I don't think it actually fixes the issue but introduces a workaround (cache mode of shared) which itself has weird behaviours. I've created an MCVE here: https://github.com/winghouchan/libsql-in-memory-transactions-broken-mcve

winghouchan avatar Dec 23 '24 21:12 winghouchan

@sivukhin, what are your thoughts as the author of #220?

winghouchan avatar Dec 23 '24 21:12 winghouchan

Yea it's more of a workaround. Was probably fixed around 0.8, my original comment was meant to say "upgrade to the latest 0.9.0 where the workaround was introduced after 0.7.0". I only use :memory: in my tests so the work around was fine for me.

khuezy avatar Dec 23 '24 22:12 khuezy