-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Feedback on using the driver with Redka #94
Comments
Thanks! The first one, I wouldn't consider a bug. I actually believe it's a misfeature of other drivers. I'll look into their implementation, but (e.g.) I don't see how it can be correctly implemented in the face of numbered ( The problem is that SQLite doesn't allow to bind parameters to To see what I mean, consider this example: _, err = db.Exec(`
insert into t1(name1) values(?, ?3);
insert into t2(name2) values(?, ?1);
`, 1, 2, 3, 4) SQLite would see But for the next statement, And worse, at the C API level, I'm not sure I can even tell the difference between numbered and positional parameters, so I can't even complain only when you mix them (which is when there really is no right answer). |
The read-only one I need to understand better. From the looks of it, it's a bug. And I haven't tested read-only connections much, so that's a great test case. Thanks again! |
Thanks for making these examples (which don't even depend on Redka!) I couldn't reproduce the read-only one "as is" (i.e., it works for me), but I'm assuming it's because With a WAL database, I can reproduce it, and it's very likely a bug with my VFS. Should be fixed in e7f8311. |
I completely agree that the "multi-exec" behavior is a design choice, not a bug. Also, mattn and modernc are quite inconsistent in this regard. My example is one of the few cases where both work and produce the same result, many other combinations will cause either mattn or modernc to fail. I'm currently using multi-execs in Redka to speed things up a bit, but I probably shouldn't.
Sure, anything to make a fellow maintainer's life a little easier. Still, I managed to miss the WAL part :)
Yep. Sorry, I missed that in the example.
Thank you! It works now 🎉 |
I'll try converting multi-execs to single-execs in Redka, re-run the tests with the ncruces driver, and let you know if anything new pops up. |
Maybe I'm missing something here, but it seems like the shared cache for memory mode doesn't work properly. If the first connection creates a table, the second connection does not see it ("sqlite3: SQL logic error: no such table: t1"). db1, err := sql.Open("sqlite3", "file:data.db?mode=memory&cache=shared")
db2, err := sql.Open("sqlite3", "file:data.db?mode=memory&cache=shared")
// create the table using the first connection
_, err = db1.Exec(`
drop table if exists t1;
create table t1(id integer primary key, name1 text);
insert into t1(name1) values('alice');
`)
// select the data using the second connection
// fails with error: sqlite3: SQL logic error: no such table: t1
var name string
err = db2.QueryRow("select name1 from t1").Scan(&name) The same code works fine with mattn/modernc drivers. |
Some benchmarks comparing mattn, modernc and ncruces drivers for
Pragmas used:
Connection strings:
|
Unfortunately, I do not support shared cache (its use is discouraged), and even if I did, I would not be able to support it for the purpose of sharing memory databases across connections, due to architecture constraints (for this driver, each connection is completely isolated from the others, like independent OS processes). The supported alternative (and increasingly recommended by SQLite developers), is the db1, err := sql.Open("sqlite3", "file:/data.db?vfs=memdb&_txlock=immediate"")
db2, err := sql.Open("sqlite3", "file:/data.db?vfs=memdb&mode=ro") The slash in the name is important, as that makes it a shared database. |
If you can share the benchmark code, I'll see if there's anything that can be improved. I don't currently support But having a good benchmark that shows its advantages would let me explore alternative implementation strategies. Edit: I suspect than might not be the biggest issue. My locking might not be as efficient. The first thing I'd try on a mac would be to rerun the benchmark with the One of the ways I got good results in this benchmark was to improve scanning of queries with many result columns, which admittedly is not the sweet spot for Redka. |
Shared cache is discouraged. The supported alternative (and increasingly recommended by SQLite developers), is the memdb VFS (which I believe is supported by all drivers). I'm sure modernc has it, and mattn also does if compiled with SQLITE_ENABLE_DESERIALIZE, which became the default in 2021. ncruces/go-sqlite3#94 (comment)
Thank you, this is very helpful! I've changed all the I've also tested Redka with the ncruces driver, and it passes all the tests. So I've added your driver to the list of supported ones in the docs.
I use the Redis benchmarking tool redis-benchmark against the Redka API as described here. I'm afraid it won't be of much use to you. Thank you very much for your help, I'm happy to see Redka working with |
That's great! I'll still look at the multiple statements, to see if I can make sense of the other drivers' take on this. At a minimum I'll open a few discussions on these incompatibilities. |
Thanks @nalgeon! I released v0.16.2 with the readonly bug fix. There's some performance gain (~10% in some benchmarks) to be had with the I'll profile your benchmark, time permitting. Also see: |
Great news, thank you Nuno! |
After trying Redka with the
ncruces/go-sqlite3
driver, I've encountered some problems. Not sure if you consider them bugs or not, but this behavior is not consistent withmattn/go-sqlite3
andmodernc.org/sqlite
(both work fine in the cases described below).Multiple statements in Exec
An Exec call with multiple parameterized queries results in a "sqlite3: multiple statements" error:
Full example (you have to run it locally, it does not run the playground)
Read-only mode
Running queries on a read-only DB (
mode=ro
connection string parameter) results in a "sqlite3: disk I/O error" error:Full example (you have to run it locally, it does not run the playground)
The text was updated successfully, but these errors were encountered: