Missing an impl for Pinger in sql driver

[jwendel]

Looking in https://github.com/ncruces/go-sqlite3/blob/main/driver/driver.go, I don't see any impl for the Ping() method, as defined in: https://pkg.go.dev/database/sql/driver#Pinger

While it doesn't entirely make sense for SQLite, I use it with the modernc driver of SQLite as a quick check that the DB file successfully opened. It would be nice to add support for this method.

[ncruces]

The modernc driver doesn't implement Pinger either.

The mattn driver does a nil check on the connection (!?).

The fact that a driver doesn't implement Ping doesn't mean you can't call sql.DB.Ping. You can.

[daenney]

As far as I can tell, db.Ping() ends up being implemented by calling pingDC() which checks if Pinger is implemented on the Conn and if so calls it. Otherwise it checks if a Conn is available: https://cs.opensource.google/go/go/+/master:src/database/sql/sql.go;l=874?q=pingDC&sq=&ss=go%2Fgo. This is also why it works with the database/sql driver that's included in the modernc build.

This program compiles and executes successfully, which wouldn't happen if Pinger had to be explicitly implemented.

package driver_test

import (
    "context"
    "fmt"
    "log"

    "github.com/ncruces/go-sqlite3/driver"
    _ "github.com/ncruces/go-sqlite3/embed"
    _ "github.com/ncruces/go-sqlite3/vfs/memdb"
)

func Example_ping() {
    db, err := driver.Open("file:/test.db?vfs=memdb", nil)
    if err != nil {
        log.Fatal(err)
    }
    defer db.Close()

    fmt.Println(db.Ping())
    fmt.Println(db.PingContext(context.Background()))

    // Output:
    // <nil>
    // <nil>
}

In both the case of modernc and this library, calling Ping() doesn't do anything. So it doesn't look like that's a good way to check if it successfully opened the database file for either library.

In the case of the database/sql driver in this library, when you call Open(...) it ends up calling the underlying connector's Connect() method which does sqlite3.Open() which eventually calls sqlite3_open_v2. I might misunderstand the call chain, but I think that's where it ends up. It gets a bit difficult to follow with going through database/sql. That'll return an error if it can't open the database, so the error returned from the Open() call should be enough to establish that fact as far as I can see. Never mind, I'm missing something in the call chain. Opening an invalid location does work, but then Ping() still correctly fails.

func Example_ping() {
    db, err := driver.Open("file:/data/bazinga.db", nil)
    if err != nil {
        log.Fatal(err)
    }
    defer db.Close()

    fmt.Println(db.Ping())
    fmt.Println(db.PingContext(context.Background()))

    // Output:
    // sqlite3: unable to open database file
    // sqlite3: unable to open database file
}

[ncruces]

Right, this is what I expected from the docs (emphasis my own):

Ping verifies a connection to the database is still alive, establishing a connection if necessary.

Once the connection is open, I'm not sure there's anything else useful I could do… If there's any SQLite API you'd like me to call, @jwendel, I'm taking suggestions. Otherwise, I'll be leaving it as it is, as it's working fine.

[jwendel]

The modernc driver doesn't implement Pinger either.

That's super weird it doesn't show up in the docs, but reading the code it does: https://gitlab.com/cznic/sqlite/-/blob/master/sqlite_go18.go?ref_type=heads#L15-24

It looks like their solution is to do an Exec("select 1").

[ncruces]

Exec("select 1") doesn't do anything, though.

If you want to see why, create a new folder, then open a database in that folder with the sqlite3 shell. The file will be created. Now delete the folder (and the file), and do select 1. It doesn't do IO, so it won't check anything.

Neither the mattn, nor the modernc pingers do anything useful in practice. I think I had Ping at one time, and removed it.

[ncruces]

Feel free to reopen if you have a better suggestion.