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

Feedback on using the driver with Redka #94

Closed
nalgeon opened this issue Jun 9, 2024 · 13 comments
Closed

Feedback on using the driver with Redka #94

nalgeon opened this issue Jun 9, 2024 · 13 comments
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@nalgeon
Copy link

nalgeon commented Jun 9, 2024

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 with mattn/go-sqlite3 and modernc.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:

db, err := sql.Open("sqlite3", "data.db")

// works fine
_, err = db.Exec(`
	drop table if exists t1;
	drop table if exists t2;
	create table t1(id integer primary key, name1 text);
	create table t2(id integer primary key, name2 text);
`)

// fails with error: sqlite3: multiple statements
args := []any{"alice", "alice"}
_, err = db.Exec(`
	insert into t1(name1) values(?);
	insert into t2(name2) values(?);
`, args...)

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:

db1, err := sql.Open("sqlite3", "file:data.db?_mutex=no&_txlock=immediate")
db2, err := sql.Open("sqlite3", "file:data.db?_mutex=no&mode=ro")

// create the table using the first (writable) connection
_, err = db1.Exec(`
	create table t1(id integer primary key, name1 text);
	insert into t1(name1) values('alice');
`)

// select the data using the second (readonly) connection
// fails with error: sqlite3: disk I/O error
var name string
err = db2.QueryRow("select name1 from t1").Scan(&name)

Full example (you have to run it locally, it does not run the playground)

@ncruces
Copy link
Owner

ncruces commented Jun 9, 2024

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 (?NNN) parameters, and especially numbered and positional (?) parameters mixed together.

The problem is that SQLite doesn't allow to bind parameters to sqlite3_exec, and sqlite3_prepare_v3 does not accept multiple statements. So, suddenly, which parameters go to which statement is up to interpretation, and IMO there is no correct answer.

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 insert into t1(name1) values(?, ?3) with arguments 1, 2, 3 and 4, and bind 1 and 3.

But for the next statement, insert into t2(name2) values(?, ?1) what should happen? If this was a single statement (one big query) this fragment would get 4 and 1. But that's actually impossible to achieve, as for an isolated fragment these both get the same index (1) so they must have the same value.

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).

https://sqlite.org/c3ref/bind_blob.html

@ncruces
Copy link
Owner

ncruces commented Jun 9, 2024

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!

@ncruces
Copy link
Owner

ncruces commented Jun 9, 2024

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 data.db was a WAL database to begin with?

With a WAL database, I can reproduce it, and it's very likely a bug with my VFS.


Should be fixed in e7f8311.

ncruces added a commit that referenced this issue Jun 9, 2024
@nalgeon
Copy link
Author

nalgeon commented Jun 10, 2024

The first one, I wouldn't consider a bug. I actually believe it's a misfeature of other drivers.

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.

Thanks for making these examples

Sure, anything to make a fellow maintainer's life a little easier. Still, I managed to miss the WAL part :)

I'm assuming it's because data.db was a WAL database to begin with

Yep. Sorry, I missed that in the example.

Should be fixed

Thank you! It works now 🎉

@nalgeon
Copy link
Author

nalgeon commented Jun 10, 2024

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.

@nalgeon
Copy link
Author

nalgeon commented Jun 10, 2024

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)

Full example

The same code works fine with mattn/modernc drivers.

@nalgeon
Copy link
Author

nalgeon commented Jun 10, 2024

Some benchmarks comparing mattn, modernc and ncruces drivers for set (two inserts, single write connection) and get (select joining two tables, 8 concurrent read connections) operations on MacBook Air M1:

mattn
SET: 26954.18 requests per second, p50=0.215 msec
GET: 104602.52 requests per second, p50=0.063 msec

modernc
SET: 20889.91 requests per second, p50=0.279 msec
GET: 83892.62 requests per second, p50=0.087 msec

ncruces
SET: 13232.76 requests per second, p50=0.431 msec
GET: 67385.45 requests per second, p50=0.119 msec

Pragmas used:

pragma journal_mode = wal;
pragma synchronous = normal;
pragma temp_store = memory;
pragma mmap_size = 268435456;
pragma foreign_keys = on;

Connection strings:

read-write db:
file:data.db?_mutex=no&_pragma=temp_store%3Dmemory&_pragma=mmap_size%3D268435456&_pragma=foreign_keys%3Don&_pragma=journal_mode%3Dwal&_pragma=synchronous%3Dnormal&_txlock=immediate

read-only db:
file:data.db?_mutex=no&_pragma=synchronous%3Dnormal&_pragma=temp_store%3Dmemory&_pragma=mmap_size%3D268435456&_pragma=foreign_keys%3Don&_pragma=journal_mode%3Dwal&mode=ro

@ncruces
Copy link
Owner

ncruces commented Jun 10, 2024

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 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.

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.

https://sqlite.org/src/file?name=src/memdb.c

https://pkg.go.dev/github.com/ncruces/go-sqlite3/vfs/memdb

@ncruces
Copy link
Owner

ncruces commented Jun 10, 2024

If you can share the benchmark code, I'll see if there's anything that can be improved.

I don't currently support mmap_size, as the way it's implemented in SQLite is… IDK.

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 sqlite3_flock build tag.

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.

nalgeon added a commit to nalgeon/redka that referenced this issue Jun 10, 2024
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)
@nalgeon
Copy link
Author

nalgeon commented Jun 10, 2024

The supported alternative (and increasingly recommended by SQLite developers), is the memdb VFS

Thank you, this is very helpful! I've changed all the mode=memory&cache=shared code to vfs=memdb, and it works like a charm.

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.

If you can share the benchmark code, I'll see if there's anything that can be improved.

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 ncruces/go-sqlite3! 🥳

@ncruces
Copy link
Owner

ncruces commented Jun 10, 2024

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.

@ncruces ncruces added bug Something isn't working documentation Improvements or additions to documentation labels Jun 11, 2024
@ncruces
Copy link
Owner

ncruces commented Jun 11, 2024

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 sqlite3_flock BSD WAL implementation. Unfortunately that's not as compliant with SQLite locking protocol (e.g. it can't be used with Litestream). I may try to get best of both worlds, but it complicates locking somewhat (and likely doesn't get the full 10%).

I'll profile your benchmark, time permitting.

Also see:

@ncruces ncruces closed this as completed Jun 11, 2024
@nalgeon
Copy link
Author

nalgeon commented Jun 12, 2024

Great news, thank you Nuno!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants