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

SQLite libsql tidying #1545

Merged
merged 6 commits into from
May 30, 2023
Merged

Conversation

itowlson
Copy link
Contributor

@itowlson itowlson commented May 26, 2023

This builds on, and includes, @rylev's #1539. The new stuff included here is:

  • Renamed the Turso implementation to libsql, per an un-PRed branch by @radu-matei
  • Broke out the rusqlite implementation into its own crate, to follow the K/V model of a base crate defining interfaces and the host component plus a crate per concrete implementation
    • In this case I used the name inproc for the rusqlite/default implementation but that is totally up for grabs
  • Hooked up the "default database" message (again using some of Radu's prior work)
  • Fixed an issue where the libsql implementation was panicking with "cannot start a runtime inside another runtime". I am entirely unsure if this is a wise implementation but the obvious fix (async all the things) was unhappy because libsql ResultSet isn't be Send, and this fix dug me out of a hole.
  • Tested spin up --sqlite and the Rust SDK against a Turso back end

Important notes:

  • At the moment, a libsql runtime config entry MUST NOT include the libsql:// prefix in the url. It should ONLY be the Turso host name.
  • The file backed location message is misleading. The current behaviour is that if you use the default it creates .spin/default.db but it prints only the directory. I have not fixed the printing because I want us to agree on the path. I made a (proposed) decision on the path and it now prints it.
  • It does not yet seem possible to set nondefault in-proc database locations in the runtime config file. It appears to treat a path as a directory, and then wants <spin-name>.db inside that directory, but it errors if the file does not already exist. Again, I can fix this but I want to get agreement on the desired behaviour first. (My mental model for this was the K/V one where you specify a file, and it creates it if it doesn't already exist.) Per Ryan's comment, I implemented K/V style behaviour.

This needs squashing and could do with a bit more tidying (indents: let us never speak of them again), but I wanted to share it for people to kick the tyres and to get feedback on the desired behaviour for the runtime config behaviour.

@rylev
Copy link
Collaborator

rylev commented May 26, 2023

Thanks so much for working on this! ❤️

I'm fine with making the path configuration match the behavior of the key-value store config.

@itowlson
Copy link
Contributor Author

Regarding this:

Fixed an issue where the libsql implementation was panicking with "cannot start a runtime inside another runtime".

After a bit more testing, it looks like this was an issue only with executing SQL during startup (via the spin up --sqlite option). When the Wasm guest issued an execute call it worked fine without my 'fix'. So the question is:

If spawning a thread for the secondary runtime is the correct solution, should that be something we do in the libsql provider execute implementation, or in the code that calls execute during startup?

I like doing it in the libsql provider because then consumers don't have to worry about it on a case-by-case basis. But it does add an overhead on every outbound libsql call - probably insignificant in timing terms compared to the network hop (though I have not measured it under any kind of load), but possibly a scaling issue if a lot of guests do it at the same time and run up a large number of OS threads.

A FANCY SCHMANCY solution would be to have the provider determine if it is within a tokio runtime: if so, it could spawn a thread for the secondary runtime, and if not it could create the secondary runtime there and then...

@itowlson
Copy link
Contributor Author

The fancy schmancy trick didn't help. Calls from the Wasm guest still show as being in a Tokio runtime, so still go down the "spawn a thread" branch even though we have seen that it's unnecessary.

@itowlson
Copy link
Contributor Author

Added a fix for inproc storage location to print the database file not just the directory, and for the runtime config file to specify the exact file path.

I provisionally changed the default filename from default.db to sqlite_db.db. The reasoning behind this was to highlight that it was associated with the database feature (similar to how K/V calls it sqlite_key_value.db). (Yes, remorseless logic would favour sqlite_sqlite.db but I wasn't sure I could pull that off.) It is easy to revert or change if it is unwelcome.

ivan@hecate:~/testing/sqlmadness$ spin up
Logging component stdio to ".spin/logs/"
Storing default key-value data to ".spin/sqlite_key_value.db"
Storing default SQLite data to ".spin/sqlite_db.db"

Serving http://127.0.0.1:3000
Available Routes:
  sqlmadness: http://127.0.0.1:3000 (wildcard)

# runtime-config.toml
[sqlite_database.default]
type = "spin" 
path = ".spindlywindly/honkytonk.cb"
# end runtime.config.toml

ivan@hecate:~/testing/sqlmadness$ spin up --runtime-config-file runtime-config.toml 
Logging component stdio to ".spin/logs/"
Storing default key-value data to ".spin/sqlite_key_value.db"
Storing default SQLite data to ".spindlywindly/honkytonk.cb"

Serving http://127.0.0.1:3000
Available Routes:
  sqlmadness: http://127.0.0.1:3000 (wildcard)

I've tested the changes for both the default default, the default mapped in the runtime config, and a nondefault database.

@itowlson
Copy link
Contributor Author

This now implements the execute_batch function so spin up --sqlite @file.sql now works which it didn't seem to before.

I have to admit I am not delighted with the libsql implementation though.

@itowlson
Copy link
Contributor Author

This is ready for review (I am happy to squash commits a bit more but wanted to leave the last couple in place in case we dislike them that much). It would be great to land it sooner rather than later, because it is already quite big, and I'd like the next lot of SQLite fixes and improvements to go into this structure, rather than being applied to the current crate and causing big merge conflicts (which is it is so big to be honest - I had fixes so I put them in here rather than waiting until it was merged or rejected!).

One thing that's lacking is tests. I think we can do at least in-proc ones in the same vein as the K/V integration tests. But again I'd kind of like to get this in then go back and battle the integration tests when they won't hold up the merge.

@radu-matei @dicej it would be great if you could take a look - I know @rylev is the authority on this but I'm not sure when he'll be available.

@radu-matei
Copy link
Member

radu-matei commented May 30, 2023

Gave this branch a try after the updates and things LGTM.
I tested:

  • no runtime configuration
  • runtime configuration pointing to a local SQLite file
  • runtime configuration without a local path, running in memory
  • LibSQL pointing to Turso

Everything seems to be working properly — the migrations file is handled properly across implementations, and @rylev's basic todo app (https://github.com/rylev/spin-todo) works great!

@itowlson
Copy link
Contributor Author

Thanks @radu-matei - I will give it some more squash, then merge, and of course iterate with @rylev when he gets back.

rylev and others added 6 commits May 31, 2023 08:11
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
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.

3 participants