Skip to content

Support for libsql and Turso#1042

Closed
avezina-ubik wants to merge 14 commits into
golang-migrate:masterfrom
avezina-ubik:master
Closed

Support for libsql and Turso#1042
avezina-ubik wants to merge 14 commits into
golang-migrate:masterfrom
avezina-ubik:master

Conversation

@avezina-ubik

Copy link
Copy Markdown

Resolves #1038

@bryanvaz

bryanvaz commented Mar 9, 2024

Copy link
Copy Markdown

@avezina-ubik Can you please correct the database connection uri in libsql/README.md and update your branch. The way your code is written, you have to specify the connection scheme after the libsql:// and does not require the query param. Specifically, your line:
database/libsql/README.md[5]:
libsql://[DATABASE].turso.io?authToken=[TOKEN]&query

Should read

libsql://https://[DATABASE].turso.io?authToken=[TOKEN]

In your unit tests you correctly insert the file:// scheme into the connection string after the libsql:// is stripped from the connection string which is why your tests pass.

Can you also add instructions on how to connect to a turso local dev/self-hosted server (e.g.: turso dev --db-file uki.db or via libsql Docker). On my local machine this is:

migrate -database libsql://http://localhost:8080

@dhui this might be another reason to move to individual CI tasks for each database. Then we can spin up sideband docker services and test cli commands against live local db instances to ensure the cli commands also work.

Cheers,
Bryan

Without the scheme in the connection string you will get the following error:

> migrate -source file://./migrations -verbose --database "libsql://rando-db-bryanvaz.turso.io?authToken=[TOKEN]&query" up
2024/03/09 18:12:13 error: unsupported URL scheme:
This driver supports only URLs that start with libsql://, file://, https://, http://, wss:// and ws://

while this works:

> migrate -source file://./migrations -verbose --database "libsql://https://rando-db-bryanvaz.turso.io?authToken=[TOKEN]" up
2024/03/09 18:15:09 Start buffering 20240309225149/u CreateCatCondoTable
2024/03/09 18:15:09 Read and execute 20240309225149/u CreateCatCondoTable
2024/03/09 18:15:09 Finished 20240309225149/u CreateCatCondoTable (read 79.775833ms, ran 122.719125ms)
2024/03/09 18:15:09 Finished after 219.338542ms
2024/03/09 18:15:09 Closing source and database

@bryanvaz bryanvaz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update and test drop command - currently not working

Comment thread database/libsql/libsql.go
return version, dirty, nil
}

func (d *LibSQL) Drop() (err error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write a test for the drop command. It currently does not work:

❯ ./scripts/migrate drop
2024/03/09 18:46:33 Are you sure you want to drop the entire database schema? [y/N]
y
2024/03/09 18:46:34 Dropping the entire database schema
2024/03/09 18:46:34 error: failed to execute SQL: DROP TABLE sqlite_sequence
SQLite error: table sqlite_sequence may not be dropped in line 0: DROP TABLE sqlite_sequence in line 0: DROP TABLE sqlite_sequence

The following table is a libsql system table and should not be dropped:

  • sqlite_sequence

Additionally to cover most usecases:

  • Please drop views using DROP VIEW before you drop tables. This list can be obtained via: SELECT name FROM sqlite_master WHERE type = 'table'
  • The order of the tables in the sqlite_master is a hint as to the order in which they were created and thus any FK constraints; therefore you should drop the tables in reverse order to avoid hitting FK constraint issues.

- Fixed Drop failing when a table has AUTOINCREMENT
- Updated README.md to explain URL scheme
- Updated README.md to add instructions on how to connect to a turso dev
@avezina-ubik

Copy link
Copy Markdown
Author

Hey @bryanvaz!

Thanks for the review. I updated the README and tried to prioritez the libsql scheme since it's the one we mostly see in the Turso docs. My guess is it would be less confusing for users, but let me know what you think.

Fixed the Drop() too

@gamebox

gamebox commented Jun 2, 2024

Copy link
Copy Markdown

Let me know if there is anything I can do to help this PR get merged. I would really like to see this happen soon

@keenanwl

keenanwl commented Jun 7, 2024

Copy link
Copy Markdown

Hey @avezina-ubik, thank you for moving this forward.

Under notes, you write that https://github.com/libsql/go-libsql is used, but it seems like you've actually got github.com/tursodatabase/libsql-client-go/libsql in go.mod.

Replacing with github.com/tursodatabase/go-libsql seems to solve my issue where I want to use embedded replicas and golang-migrate as a library, but hit: sql: Register called twice for driver libsql

@avezina-ubik

Copy link
Copy Markdown
Author

Hi @keenanwl

I will update the README soon with github.com/tursodatabase/libsql-client-go.

From what I understand (though I'm a bit confused by Turso's documentation):

  • github.com/tursodatabase/libsql-client-go is used to interact with a libsql server and work with local .db files requiring you to import a SQLite driver.
  • github.com/tursodatabase/go-libsql allows you to sync your embedded replica with a libsql server, interact with a libsql server, and work with local .db files without needing to import a SQLite driver

The thing is, github.com/tursodatabase/go-libsql needs CGO_ENABLED=1 in order to link to libsql static library (see here)
whereas github.com/tursodatabase/libsql-client-go seems to be using net/http or nhooyr.io/websocket to communicate with the libsql server (see here).

Correct me if I am wrong, but this project seems to require CGO_ENABLED=0:

cd ./cmd/migrate && CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o ../../cli/build/migrate.linux-amd64 -ldflags='-X main.Version=$(VERSION) -extldflags "-static"' -tags '$(DATABASE) $(SOURCE)' .

So this is why I went with github.com/tursodatabase/libsql-client-go

As for your issue I am not sure I understand quite well the problem since you can update your schema pointing at Turso's remote URL:

./migrate -database libsql://libsql://my-db.turso.io?auth_token=<tok> -path migrations/ up

Your embedded replicas will then receive WALs and sync the changes.

@avezina-ubik

Copy link
Copy Markdown
Author

Hey guys,

I noticed that using this driver to interact with a file (e.g. ./migrate -database libsql://file://./my.db -path migrations/ up) does not work right now.

Besides a URL parsing issue (which can be easily fixed), there's also a dependency on a SQLite driver.

Would it be acceptable for this database implementation to only support interacting with libsql servers? Here are my thoughts:

  • 99% of people using migrate with libsql will be using Turso (quote needed)
  • I can't find any scenarios where opening a libsql .db file through a SQLite driver would be beneficial.
  • Importing a SQLite driver like modernc.org/sqlite would make this database a hybrid of libsql and SQLite, which could be confusing and complicate testing.

My suggestion (see last commit) :

  1. Return an error when URL is libsql://file://...
  2. Remove file:// from the README

If you prefer, I can import a SQLite driver and make it work.

Regards

@keenanwl

keenanwl commented Jun 7, 2024

Copy link
Copy Markdown

Hi @avezina-ubik. Sounds reasonable.

Looks like you are correct about cgo being disabled for the CLI - I didn't realize that was an issue, since I don't use it.

However, when imported as a library, then sqlite3 migrations have been working fine for me (using: 1) and I assumed libsql would be a drop-in replacement (e.g. no sql.Register("libsql", Driver{}) conflicts.

If indeed most users are working with the CLI or Turso-remote, then yes, I would also agree that prioritizing github.com/tursodatabase/libsql-client-go is the way to go. In that case I will find an alternative.

@avezina-ubik

Copy link
Copy Markdown
Author

Oh! I understand your problem now @keenanwl. I assumed you were using the CLI.

Since you are already importing github.com/tursodatabase/go-libsql from somewhere in your source, you are getting the infamous sql: Register called twice for driver libsql because this implementation imports github.com/tursodatabase/libsql-client-go and both libraries register a libsql driver.

I thought about filing an issue with https://github.com/tursodatabase/libsql-client-go, so they could avoid registering the driver if one is already there, but this would lead to unpredictable behavior since the order of init cannot be determined.

I am pretty sure you already thought about that, but to avoid importing github.com/tursodatabase/libsql-client-go you could copy/paste my implementation in your codebase and change the import to github.com/tursodatabase/go-libsql. You then replace _ "github.com/golang-migrate/migrate/v4/database/libsql" with the package in your codebase. It's quite dirty, but cleaner than having a separate bin

@L-four

L-four commented Jul 14, 2024

Copy link
Copy Markdown

It's not the cleanest but we could add a build tag to switch between github.com/tursodatabase/libsql-client-go/libsql and github.com/tursodatabase/libsql-client-go

@ainsleyclark

Copy link
Copy Markdown

Hey @avezina-ubik
Thanks for this PR, do you know if it’s likely to get merged soon?

@avezina-ubik

Copy link
Copy Markdown
Author

Hi @ainsleyclark

Don't think it's gonna happen to be honest.

Best bet would be to fork a build and have you own Docker image. I am still using something like this : #1038 (comment) and it's been working great.

@RafaelZasas

Copy link
Copy Markdown

@avezina-ubik Is there a need to have a libsql driver since its fully compatible with sqlite? I was using the sqlite driver and it worked just fine.

The thing that would be great however is to add support for the new turso rewrite of sqlite which has features that are not backwards compatible like custom types.

@avezina-ubik

Copy link
Copy Markdown
Author

Hey @RafaelZasas,
given the amount of time that has passed, I'm no longer maintaining this PR or tracking this issue. I’ll leave it up to the maintainers to decide if they want to merge 'as-is' or close it.

@RafaelZasas

Copy link
Copy Markdown

@avezina-ubik If you or anyone else is interested, I made a PR to add turso as a driver. I know this PR is for libsql, but I assume that a good number of people who were using libsql has, or soon will migrate to using turso.
#1393

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

Successfully merging this pull request may close these issues.

Support for libsql and Turso

7 participants