Using driver/sql hangs database and other oddities

[tie]

I can reproduce mattn/go-sqlite3#204 with minimal changes.

main1.go

// https://github.com/mattn/go-sqlite3/issues/705

package main

import (
	"database/sql"

	_ "github.com/genjidb/genji/sql/driver"
)

func main() {
	db, err := sql.Open("genji", ":memory:")
	if err != nil {
		panic(err)
	}
	_, err = db.Exec("CREATE TABLE m")
	if err != nil {
		panic(err)
	}
	_, err = db.Query("SELECT * FROM m") // note that I didn’t close the rows
	if err != nil {
		panic(err)
	}
	_, err = db.Query("SELECT * FROM m")
	if err != nil {
		panic(err) // table not found
	}
	panic("unreachable")
}

main2.go

package main

import (
	"context"
	"database/sql"

	_ "github.com/genjidb/genji/sql/driver"
)

func main() {
	ctx := context.Background()

	db, err := sql.Open("genji", "test.db")
	if err != nil {
		panic(err)
	}
	db.Conn(ctx)
	db.Conn(ctx)
	panic("unreachable")
}

I assume SQLite releases db locks when they are not needed. But this issue gets worse with Genji since BoltDB blocks on db lock (unless a timeout was specified, which is not currently possible with sql/driver), and BadgerDB I believe just returns an error. That is, things go terribly wrong when sql.DB attempts to open a new sql.Conn.

As a temporary workaround it’s possible to call SetMaxOpenConns(1) on *sql.DB from the user’s code. This should work just fine for in-memory and bolt engines, but afaik badger supports concurrent transactions whereas single driver.Conn can’t be used concurrently.

I’ve experimented a bit with possible APIs to fix those issues, and I think the best option would be to fix this use case on the Go side by allowing driver.Connector to implement optional io.Closer interface and close it in sql.DB.Close. Then driver.Connector would be able to hold genji.DB object and release the resources when sql.DB is no longer needed.

Alternatively, it’s possible to do reference counting on genji.DB in driver.Connector and driver.Conn, and have a special case for in-memory engines. That has a benefit of releasing the database handle when idle (e.g. so that other processes may perform some maintenance tasks).

• Also, it looks like driver.Conn is reusing itself as driver.Tx for transactions (lines 117–123). This won’t work as expected if we begin multiple transactions on a single connection. https://github.com/genjidb/genji/blob/112fd24e38b07590fd077be043837e0425731a8a/sql/driver/driver.go#L108-L124

[asdine]

Thank you for the detailed issue @tie! The sql/driver package was written quite some time ago and my focus was on the core until now. There is currently no concept of connection in Genji, so that what makes this sql/driver implementation a bit odd.

I don't think there should be a special case for the in-memory engine, but rather make sure that every instance of sql.DB represent an access to the same genji.DB instance, regardless of the engine used.

Concerning your proposals, I think they both make sense, I will look into them and try to propose a fix for the next release.