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

[#24] Use https://gitlab.com/cznic/sqlite to be cgo-free #26

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lispyclouds
Copy link
Member

Also addresses #25

@lispyclouds
Copy link
Member Author

  • this changes the base version to 1.22
  • updates all deps
  • passes all tests including extensions like fts5

@lispyclouds
Copy link
Member Author

lispyclouds commented Feb 1, 2025

we dont use go-sqlite3 anymore, does that need a change of pod name? technically its its still go and supports sqlite3 😆

@lispyclouds
Copy link
Member Author

lispyclouds commented Feb 1, 2025

can just use one CI like circle to make all binaries with the trade-off of not testing binaries in their native envs.

@lispyclouds
Copy link
Member Author

Circle CI failure is due to the install script installing an older Go version

@lispyclouds lispyclouds changed the title [discussions/#24] Use https://gitlab.com/cznic/sqlite to be cgo-free [#24] Use https://gitlab.com/cznic/sqlite to be cgo-free Feb 1, 2025
@lispyclouds
Copy link
Member Author

Should be in a usable state. Lemme know what you think @borkdude

@borkdude
Copy link
Contributor

borkdude commented Feb 1, 2025

How would this affect performance? Are there any other future limitations that might come with switching?

@lispyclouds
Copy link
Member Author

Lemme read up and come back on the perf bit. As for the future limitations, the way this works is translating the C code to Go with some mods, meaning it should be at feature parity with the C impl and it could go out of sync for a bit when a new thing comes out. This is an issue in the current go-sqlite3 too as that embeds the C things and needs another release too. Will hammock more.

@lispyclouds
Copy link
Member Author

This lib seems to have good usage, recommendations and support hence makes me a more convinced.

@lispyclouds
Copy link
Member Author

lispyclouds commented Feb 1, 2025

re perf, based on https://github.com/cvilsmeier/go-sqlite-bench this lib is pretty slow (i was expecting it) but https://github.com/zombiezen/go-sqlite looks interesting, but its not a database/sql driver meaning our code would need more changes.

@lispyclouds
Copy link
Member Author

lispyclouds commented Feb 1, 2025

https://datastation.multiprocess.io/blog/2022-05-12-sqlite-in-go-with-and-without-cgo.html
tldr; is between twice as slow and 10% slower for both small datasets and large, for INSERTs and SELECTs.

looks like if its perf critical, raw sqlite is to be used otherwise for smaller things the perf tradeoff is worth the cgo drop.

Given that things like JVM easily outperforms Go when it comes to compute, not surprising.

@borkdude
Copy link
Contributor

borkdude commented Feb 1, 2025

looks like if its perf critical, raw sqlite is to be used otherwise for smaller things the perf tradeoff is worth the cgo drop.

I do not follow this reasoning. Why would you use this pod only for "small things", it's suited for "big things" as well. The pod overhead is only about the data being transferred, not about what's happening inside of sqlite in terms of query performance.

@lispyclouds
Copy link
Member Author

Maybe its better worded in the second article:

Based on these results, if your workload has solely small datasets (i.e. small business apps) the tradeoff allowing you to avoid cgo could be worth it. Otherwise if you care strongly about performance you'll be better off with the real SQLite

Maybe we can do it that way, keep this pod as is and another one which is cgo free, make the choice and tradeoffs explicit for users?

@borkdude
Copy link
Contributor

borkdude commented Feb 1, 2025

Maybe a new project is the safest and sanest way to go, but on the other hand I also don't feel like maintaining multiple sqlite projects. Perhaps we can wait a bit more until someone comes complaining about the dynamic linking stuff again, then we'll merge this?

@lispyclouds
Copy link
Member Author

Makes sense. I can do more research on how other libs etc behave and keep this updated.

@lispyclouds
Copy link
Member Author

The perf diff is quite big, was expecting but not that much.

@borkdude
Copy link
Contributor

borkdude commented Feb 1, 2025

Btw are you also aware of this one? https://changelog.com/podcast/626. It came onto my radar today in my podcast feed, haven't listened to it.

@lispyclouds
Copy link
Member Author

Didn't listen to this particular podcast but quite familiar with Turso via ThePrimeagen. The product is very cool, specially things like a db per user model. Will give this a listen too.

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.

2 participants