stdlib driver registration seemingly removed/moved.

[heedson]

From what I can tell, the method I've used to set up wrappers such as https://github.com/opencensus-integrations/ocsql/blob/master/driver.go#L53 to wrap the pgx driver in a logging driver, is no longer implemented.

The comments from the following link show that the functionality is still included in the comments so wonder if I'm missing if it has moved or the comments have not been updated: https://github.com/jackc/pgx/blob/master/stdlib/sql.go#L32

If it has been removed on purpose, consider readding an alternative or potentially investigate methods in which to allow others to wrap the pgx drivers with their own (as my example does/seems to do).

[jackc]

RegisterDriverConfig was a hack to work around the inability to configure pgx options like TLS configuration since sql.Open only accepted a connection string. However, in Go 1.10 this was solved by sql.OpenDB. Since pgx v4 requires Go 1.11+ stdlib.RegisterDriverConfig was removed in favor of using stdlib.OpenDB. The reference to RegisterDriverConfig in the docs is obsolete and should be removed.

I believe OpenDB should allow the wrapping behavior you desire.

[heedson]

Thanks for the speedy reply.

So what I'm wondering now is that stdlib.OpenDB still defines the default driver for the user. Perhaps this isn't your issue and maybe it's the wrapping method's issue but I'm not quite sure how I'd go about using stdlib.OpenDB with a different/wrapped driver.

An example or suggestion for

I believe OpenDB should allow the wrapping behavior you desire.

would be greatly appreciated.

[heedson]

Just for a concrete example, here is the situation I would use this wrapper to make the *sql.DB:

    // Register the ocsql wrapper for the provided postgres driver.
    driverName, err := ocsql.Register("pgx", ocsql.WithAllTraceOptions())
    if err != nil {
        return nil, err
    }
    driverConfig := stdlib.DriverConfig{
        ConnConfig: pgx.ConnConfig{
            Logger: logrusadapter.NewLogger(logger),
        },
    }
    stdlib.RegisterDriverConfig(&driverConfig)
    db, err := sql.Open(driverName, driverConfig.ConnectionString(pgURL.String()))
    if err != nil {
        return nil, err
    }
    return db, nil

This is the code from pre-v4, thanks.

[jackc]

Hmm... So OpenDB returns a *sql.DB that could be wrapped. But ocsql is trying to wrap at a lower level.

I see two options. If we made stdlib.connector public then you could use ocsql.WrapConnector. Otherwise we'd have to restore RegisterDriverConfig for wrapping based on driver name.

[heedson]

I don't mind which, if any. Just wanted to raise it in case you find it valuable to support this, when given a new example use case.

If you have a preference of one or the other, I can get to work on a PR to re-add or export stdlib.Connector, if you'd like.

[jackc]

Probably restoring RegisterDriverConfig would be most convenient for people relying on that behavior.

[heedson]

Sorry for delay, I'll get a PR up if it looks simple enough.

Thanks.

[heedson]

I looked through the differences from before and I'm not entirely sure what should be readded to support this functionality. Without knowing the reasons for removal of some used features, I'm not confident in readding them.

How would you like this to progress?

[jackc]

Ugh... yeah, it looks like I ripped that out in the same commit as other v3 to v4 changes. So much for a simple git revert.

...

Hmmm... So looking at the difference between what is in the latest v3 and v4 it looks like the major complicating factor is that ConnConfig.Merge was removed. This was on purpose as there were some ambiguities when merging certain fields -- especially relating to fields where the zero value could be meaningful or it could mean it was just never assigned.

I don't want to reintroduce ConnConfig.Merge, but something like that is needed for RegisterDriverConfig. Maybe something where there is an explicit list of config fields that are always and only used from the registered config. (or vice versa -- fields that only the connection string passed to sql.Open can set). That would eliminate the ambiguity.

...

Or maybe even better would be that if a registered driver config is used it must contain all connection settings. No merging at all from the additional params passed through the sql.Open string.

Potentially this could end up with usage like the following:

connConfig, _ := pgx.ParseConfig(...)
// Do whatever customization to connConfig

// RegisterDriverConfig now returns a magic connection string instead of an object that ConnectionString is called on.
connString := stdlib.RegisterDriverConfig(connConfig)

sql.Open("pgx", connString)

I think this would restore the functionality that was lost in v3 to v4, but do it in a cleaner and simpler way.

[heedson]

I like the second example you provided. Seems nice and clean, as you described. I think it seems like the best way forward for the time being. I'll update on any issues I run into (related to removed code), if I run in to them.

[sidh]

Hit this snag too and its a blocker for us as we use PreferSimpleProtocol = true as default.

In our code we often have functions returning default whatever (like default configs) that user can customize. This avoids merge logic and allows users to customize just what they want.

[jackc]

I just pushed an implementation of the second idea. you can now do things like this:

connConfig, _ := pgx.ParseConfig(os.Getenv("DATABASE_URL"))
connConfig.Logger = myLogger
connStr := stdlib.RegisterConnConfig(connConfig)
db, _ := sql.Open("pgx", connStr)