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

Fix Transactions in sql-libsql #3720

Merged
merged 18 commits into from
Oct 8, 2024
Merged

Conversation

jkonowitch
Copy link
Contributor

@jkonowitch jkonowitch commented Oct 2, 2024

Type

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

The libsql sdk uses a non-standard method of initiating transaction connections that is not compatible with @effect/sql. It expects to fully manage sending BEGIN, COMMIT, ROLLBACK statements as well as initiating new connections because under the hood it uses a custom websocket protocol. See

Therefore, I believe we should treat libsql as we do D1, as a package that does not support transactions. Users can batch multiple statements together using the batch API if needed: https://docs.turso.tech/sdk/ts/reference#batch-transactions

@thewilkybarkid realized that withTransaction can be easily overriden, so we are taking that approach.

Related

Copy link

changeset-bot bot commented Oct 2, 2024

🦋 Changeset detected

Latest commit: e9727ca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@effect/sql-libsql Minor
@effect/sql-mssql Patch
@effect/sql Patch
@effect/cluster-workflow Patch
@effect/cluster Patch
@effect/sql-d1 Patch
@effect/sql-drizzle Patch
@effect/sql-kysely Patch
@effect/sql-mysql2 Patch
@effect/sql-pg Patch
@effect/sql-sqlite-bun Patch
@effect/sql-sqlite-node Patch
@effect/sql-sqlite-react-native Patch
@effect/sql-sqlite-wasm Patch
@effect/cluster-node Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@thewilkybarkid
Copy link
Contributor

I've just hit the same problem when using a libSQL Server (i.e. Turso); it works fine with a local SQLite (hence the tests passing). I've been looking at sql-mssql as it overrides withTransaction, but I'm unsure if doing that would be enough.

@jkonowitch
Copy link
Contributor Author

jkonowitch commented Oct 2, 2024

I've just hit the same problem when using a libSQL Server (i.e. Turso); it works fine with a local SQLite (hence the tests passing). I've been looking at sql-mssql as it overrides withTransaction, but I'm unsure if doing that would be enough.

Oh very interesting. I didn't realize overrides were already an established pattern here. I mean, that provides some hope of a solution. Do you have any example code you've tried so far?

The other change I want to make to this package is to allow a live client to be provided instead of config. The reason being is that the Turso client is doing replication frame tracking as well, so if you want to use the batch API or otherwise interact with the database and get "read-your-writes semantics", you would need to share this client with other parts of your code.

/**
 * @category models
 * @since 1.0.0
 */
export type LibsqlClientConfig =
  & {
    readonly spanAttributes?: Record<string, unknown> | undefined
    readonly transformResultNames?: ((str: string) => string) | undefined
    readonly transformQueryNames?: ((str: string) => string) | undefined
  }
  & ({
    _tag: "Config"
    /** The database URL.
     *
     * The client supports `libsql:`, `http:`/`https:`, `ws:`/`wss:` and `file:` URL. For more infomation,
     * please refer to the project README:
     *
     * https://github.com/libsql/libsql-client-ts#supported-urls
     */
    readonly url: string
    /** Authentication token for the database. */
    readonly authToken?: string | undefined
    /** etc... */
  } | {
    _tag: "LiveClient"
    liveClient: Libsql.Client
  })

Oh, and FYI I added the live libsql docker image to the tests now via testcontainers so we should not get any more false positive tests.

@thewilkybarkid
Copy link
Contributor

Oh very interesting. I didn't realize overrides were already an established pattern here. I mean, that provides some hope of a solution. Do you have any example code you've tried so far?

Not yet, but it's a blocker for us at the moment so I'll be looking shortly to see if I can figure it out. (I'm new to LibSQL too!)

The other change I want to make to this package is to allow a live client to be provided instead of config. The reason being is that the Turso client is doing replication frame tracking as well, so if you want to use the batch API or otherwise interact with the database and get "read-your-writes semantics", you would need to share this client with other parts of your code.

👍 Could make the LibSQL client available as a service too; probably outside the scope of this package though so maybe a separate makeFromClient constructor rather than differing config?

Oh, and FYI I added the live libsql docker image to the tests now via testcontainers so we should not get any more false positive tests.

Fantastic! Might be worth running them against both LibSQL and SQLite as there seem to be more differences than I originally anticipated.

@thewilkybarkid
Copy link
Contributor

I suspect there is a way to get it to work, but it's beyond my Effect knowledge. It seems that withTransaction could be overridden with something that replaces the Libsql.Client with a Libsql.Transaction for that scope (something about FiberRef?), then LibsqlConnection's run/runRaw could get it at runtime rather than always using the original Libsql.Client.

@thewilkybarkid
Copy link
Contributor

I've hacked something together where transactions seem to work against both LibSQL Server and SQLite: thewilkybarkid@047ca25. There's probably plenty of gotchas though, and it doesn't support nested transactions/savepoints.

@jkonowitch
Copy link
Contributor Author

Awesome work! I am out of the office today but will take a deeper look tomorrow. Feel free to hit me up on discord as well - @jkonowitch

@tim-smart
Copy link
Member

How does libsql handle nested transactions? I didn't see any mention of savepoints.

@thewilkybarkid
Copy link
Contributor

thewilkybarkid commented Oct 4, 2024

I'm not sure the JS client does at the moment, can't see any references to it. I've opened tursodatabase/libsql-client-ts#272.

@jkonowitch
Copy link
Contributor Author

jkonowitch commented Oct 4, 2024

Re savepoints - once you have initiated a transaction with the client, you proceed as normal and can execute SAVEPOINT and ROLLBACK TO commands as normal against the tx connection. I have tested this:

import { createClient } from "@libsql/client";

const client = createClient({ url: 'http://localhost:8080' });

const tx = await client.transaction('write');

await tx.execute(`
-- Set a savepoint
SAVEPOINT sp1;
`);

await tx.execute(`
-- Insert a new user
INSERT INTO users (name) VALUES ('Alice');
`)

await tx.execute(`
  -- Set another savepoint
SAVEPOINT sp2;
`)

await tx.execute(`-- Insert another user
INSERT INTO users (name) VALUES ('Bob');
`)

await tx.execute(`-- Rollback to the second savepoint (sp2) to undo the last insert
ROLLBACK TO sp2;
`)
await tx.commit();
→  select * from users;
ID     NAME      
3      Alice

@jkonowitch
Copy link
Contributor Author

I've hacked something together where transactions seem to work against both LibSQL Server and SQLite: thewilkybarkid@047ca25. There's probably plenty of gotchas though, and it doesn't support nested transactions/savepoints.

This looks like a great start to me. Do you want to make a PR into my branch, have me copy it over, or open a new PR?

@thewilkybarkid
Copy link
Contributor

Happy for you to copy it over if you have time @jkonowitch!

@jkonowitch
Copy link
Contributor Author

Happy for you to copy it over if you have time @jkonowitch!

Just did - added you as --author 46cdfb0

I will add tests / implementation for savepoints.

@jkonowitch jkonowitch changed the title Remove Transactions from sql-libsql Fix Transactions in sql-libsql Oct 4, 2024
@jkonowitch
Copy link
Contributor Author

@tim-smart @thewilkybarkid I think this is now ready for review.

@tim-smart
Copy link
Member

Thanks!

I ended up adding SqlClient.makeWithTransaction to ensure consistency with tracing etc.

@tim-smart tim-smart merged commit e0a5dad into Effect-TS:main Oct 8, 2024
9 of 10 checks passed
@github-actions github-actions bot mentioned this pull request Oct 8, 2024
@jkonowitch jkonowitch deleted the remove-libsql-tx branch October 8, 2024 23:50
@thewilkybarkid
Copy link
Contributor

Thanks both. 🙂

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

Successfully merging this pull request may close these issues.

Transactions in sql-libsql do not work
3 participants