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

proposal: database/sql: allow access to raw driver.Conn #29835

Closed
rittneje opened this issue Jan 19, 2019 · 12 comments

Comments

Projects
None yet
5 participants
@rittneje
Copy link

commented Jan 19, 2019

It would be really nice if the new sql.Conn type had a method to access the underlying driver.Conn, similar to how net.TCPConn has the SyscallConn method. This would allow access to custom driver methods that don't map to the standard driver.Conn interface. For example, the mattn driver for SQLite exposes a Backup method on its *SQLiteConn object, but you cannot really call it once you are in the database/sql world.

Specifically, I propose adding a new method to sql.Conn as follows:

func (c *Conn) RawControl(func(driver.Conn) error) error

The driver.Conn provided to the callback must only be used within the callback. (Just like the fd cannot be used outside the callback for syscall.RawConn.) Until the callback function returns, that particular driver.Conn is "in use" and cannot be used concurrently by any other goroutine.

An equivalent method could also be added to sql.DB itself, but that doesn't seem necessary.

Sample use case:

db, _ := sql.Open("sqlite3", "...")
...
c, err := db.Conn(ctx)
if err != nil {
    panic(err)
}
defer c.Close()

err = c.RawControl(func (rc driver.Conn) error {
    c2, err := new(sqlite3.Driver).Open("backup.sqlite")
    if err != nil {
        return err
    }
    defer c2.Close()

    c2.Backup("main", rc.(*sqlite3.Conn), "main")
    ...
})
...

@gopherbot gopherbot added this to the Proposal milestone Jan 19, 2019

@gopherbot gopherbot added the Proposal label Jan 19, 2019

@kardianos

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2019

That's an interesting idea. Ill need to think about it.

@kardianos kardianos self-assigned this Jan 20, 2019

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

ping @kardianos

@kardianos

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Thanks for the ping.
There are a number of drivers which wouldn't benefit from this currently:

But there are some drivers where this could be a useful escape hatch:

The various features that may be aided in this are:

  • SQLite management
  • Connection Notifications (work around exist)
  • Bulk Data Loading APIs (work around exist)

One reason to having this escape hatch is it allows a unified connection pool. However, it also allows users to shoot themselves in the foot easier and may be a source of reported "bugs" in the future if accepted.

I think one option SQLite has is to put these management options on the a database/sql#Connector type.

I'm declining this proposal for now. If other driver authors come forward with needs that must be put on connection as driver specific, we can re-open this proposal for consideration.

@rittneje

This comment has been minimized.

Copy link
Author

commented Feb 6, 2019

@kardianos I don't entirely understand your counter-proposal to use "database/sql#Connector". Are you referring to the existing driver.Connector type, or a hypothetical new sql.Connector type? Assuming the former, I don't see how that could actually work, since my client code cannot take the connection lock that the standard library uses. Consequently, such code cannot be used in a thread-safe manner without me needing to add even more locks to the picture.

"One reason to having this escape hatch is it allows a unified connection pool." Keep in mind that the database/sql package is useful beyond just connection pooling. It also makes the various driver.Conn methods much easier to use - think of iterating through a result set for example.

"However, it also allows users to shoot themselves in the foot easier and may be a source of reported "bugs" in the future if accepted." That's fair, but I'd argue the same could be said of syscall.RawConn and even sql.Conn.

@kardianos

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Hi @rittneje

Are you using this driver for sqlite? https://godoc.org/github.com/mattn/go-sqlite3

Let's look at the methods on the Conn:

// All of these methods don't actually work on a connection level!
// All of these methods work on a database file.
func (c *SQLiteConn) AuthEnabled() (exists bool)
func (c *SQLiteConn) AuthUserAdd(username, password string, admin bool) error
func (c *SQLiteConn) AuthUserChange(username, password string, admin bool) error
func (c *SQLiteConn) AuthUserDelete(username string) error
func (c *SQLiteConn) Authenticate(username, password string) error
func (c *SQLiteConn) AutoCommit() bool
func (c *SQLiteConn) Backup(dest string, conn *SQLiteConn, src string) (*SQLiteBackup, error)
func (c *SQLiteConn) GetFilename(schemaName string) string
func (c *SQLiteConn) GetLimit(id int) int
func (c *SQLiteConn) LoadExtension(lib string, entry string) error
func (c *SQLiteConn) RegisterAggregator(name string, impl interface{}, pure bool) error
func (c *SQLiteConn) RegisterAuthorizer(callback func(int, string, string, string) int)
func (c *SQLiteConn) RegisterCollation(name string, cmp func(string, string) int) error
func (c *SQLiteConn) RegisterCommitHook(callback func() int)
func (c *SQLiteConn) RegisterFunc(name string, impl interface{}, pure bool) error
func (c *SQLiteConn) RegisterRollbackHook(callback func())
func (c *SQLiteConn) RegisterUpdateHook(callback func(int, string, string, int64))
func (c *SQLiteConn) SetLimit(id int, newVal int) int

// All of these methods work on a logical "connection".
// These match driver.Conn exactly.
func (c *SQLiteConn) Begin() (driver.Tx, error)
func (c *SQLiteConn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error)
func (c *SQLiteConn) Close() error
func (c *SQLiteConn) Exec(query string, args []driver.Value) (driver.Result, error)
func (c *SQLiteConn) ExecContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Result, error)
func (c *SQLiteConn) Ping(ctx context.Context) error
func (c *SQLiteConn) Prepare(query string) (driver.Stmt, error)
func (c *SQLiteConn) PrepareContext(ctx context.Context, query string) (driver.Stmt, error)
func (c *SQLiteConn) Query(query string, args []driver.Value) (driver.Rows, error)
func (c *SQLiteConn) QueryContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Rows, error)

I admit that SQLite has a weak concept of "database" vs "connection". Many users of SQLite limit the connection pool to a single connection. If we took your above approach, then an online backup command would lockout all other queries until it completed. Other functions, such as the "RegisterX" functions should probably be called prior to any query is called.

Are you referring to the existing driver.Connector type, or a hypothetical new sql.Connector type?

driver.Connector is an interface that is provided to sql.OpenDB. Here is an example of a Connector that currently works that provides a richer interface to the database: https://godoc.org/github.com/denisenkom/go-mssqldb#Connector . Most configuration in this instance are just properties. But in SQLite's case the connector could also contain various methods that are currently located on the Conn. I don't think the mutex issue you raised would actually be an issue.

@rittneje

This comment has been minimized.

Copy link
Author

commented Feb 7, 2019

@kardianos I believe you are mistaken about how SQLite works. There actually is a strong distinction between a connection (or context) and the actual database (file). Like any other DBMS, the connection allows you to interact with the database in various ways. There's the typical select/insert/update/delete operations, as well as some more "meta" operations, like adding/removing users, etc.

Going through a few of your examples:

  • Authenticate authenticates this connection against the database, and is thus a per-connection concept
  • AutoCommit fetches the current commit mode for this connection, and is thus a per-connection concept
  • GetFilename fetches the filename for a schema, and since this includes attached databases, it is a per-connection concept
  • LoadExtension loads an extension into a connection, and is thus a per-connection concept
  • RegisterAggregator registers a custom aggregator function into a connection, and is thus a per-connection concept
  • GetLimit/SetLimit set runtime limits for a connection, and are thus per-connection concepts

Many users of SQLite limit the connection pool to a single connection. If we took your above approach, then an online backup command would lockout all other queries until it completed.

Indeed, in the specific case of backup, holding the lock for the whole thing is unnecessary (but there are ways of dealing with that). However, for most of the other "special" SQLite methods, this locking is necessary, especially because the contract on driver.Conn is that it won't be used by multiple goroutines at once.

Other functions, such as the "RegisterX" functions should probably be called prior to any query is called.

I agree that in the common case, the intent is to register such things at the pool level, so having this be the job of the driver.Connector makes sense.

@kardianos

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

I'm re-opening this issue. Upon further inquery, an Oracle SQL driver maintainer would also be interested in such an interface as well. I think I would call the function func (c *Conn) Raw(func(driverconn driver.Conn) error) error.

@kardianos

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Upon further reflection, I would want to avoid locking in a specific interface such as driver.Conn to this method, as that would be difficult to satisfy if we were to release a database/sql/v2 that backed the v1 interface with a shim. So such a function would look like something like this:

func (c *Conn) Raw(f func(driverConn interface{}) error) error

Also, I don't expect or want driver.Conn methods to be called, we would expect someone to type assert this to a specific driver instance anyway for specific functionality.

The other way I could see spelling this type of function would be as follows:

// Raw returns a raw driver connection behind the Conn that may be type asserted to the specific
// driver type for specialized operations. Until close is called, all other operations on Conn will
// block. After close is called, the raw driverConn must not be used.
//
// Most users should not use this interface.
//
//    conn, err := db.Conn(ctx)
//    if err != nil {
//        return err
//    }
//    defer conn.Close()
//    raw, close := conn.Raw()
//    defer close()
//    if dc, ok := raw.(mydriver.Interface); ok {
//        // Use "dc".
//    }
func (c *Conn) Raw() (driverConn interface{}, close func())
@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

func (c *Conn) Raw(f func(driverConn interface{}) error) error

sounds good. Let's do that unless anyone objects. Will leave open for another week.

@andybons

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Looks like no one objects. Accepted.

@andybons andybons modified the milestones: Proposal, Go1.13 Apr 17, 2019

@kardianos

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

I'll implement within a few days.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 26, 2019

Change https://golang.org/cl/174182 mentions this issue: database/sql: add Conn.Raw to expose the driver Conn safely

@gopherbot gopherbot closed this in dc63b59 Jun 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.