Skip to content
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

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

Open
epatey opened this issue Jun 24, 2024 · 10 comments
Open

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

epatey opened this issue Jun 24, 2024 · 10 comments

Comments

@epatey
Copy link

epatey commented Jun 24, 2024

#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
Copy link
Author

epatey commented Jun 24, 2024

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;
}

@erkannt
Copy link

erkannt commented Jun 28, 2024

Related? tursodatabase/libsql#1411

@khuezy
Copy link

khuezy commented Aug 15, 2024

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

thewilkybarkid added a commit to PREreview/prereview.org that referenced this issue Oct 9, 2024
The tests began to fail when re-enabling transactions as they're incompatible with in-memory SQLite databases. It should be possible to use `file::memory:?cache=shared` rather than `:memory:`, but this also doesn't entirely. To avoid spending more time on this problem, this commit uses temporary file-based databases in the tests.

Refs #1973, 3acf814, tursodatabase/libsql-client-ts#229, https://www.sqlite.org/inmemorydb.html
@winghouchan
Copy link

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.

@khuezy
Copy link

khuezy commented Dec 23, 2024

@winghouchan Hi, my comment wasn't clear, the OP was in June so the diff was on an older version:
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.

@winghouchan
Copy link

@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
Copy link

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

@winghouchan
Copy link

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
Copy link

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

@khuezy
Copy link

khuezy commented Dec 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants