Implement database/sql/driver.DriverContext

[dcormier]

In order to fully instrument SQL operations (including new conns) by wrapping this driver with something like github.com/luna-duclos/instrumentedsql, this driver needs to implement database/sql/driver.DriverContext.

Now it does.

[LasTshaMAN]

LGTM!

[sam-github]

The command "PQTEST_BINARY_PARAMETERS=yes go test -race -v ./..." exited with 1.

[dcormier]

The command "PQTEST_BINARY_PARAMETERS=yes go test -race -v ./..." exited with 1.

The failure is:

--- FAIL: TestRuntimeParameters (0.00s)
    conn_test.go:1487: client_encoding must be absent or 'UTF8'

So, the docs state that that's expected:

Note that the connection parameter client_encoding (which sets the text encoding for the connection) may be set but must be "UTF8", matching with the same rules as Postgres. It is an error to provide any other value.

Source

That test uses a value for client_encoding other than UTF8. So, this appears to be an expected failure.

This is a test that existed before. The question is: why didn't it fail before?

Perhaps someone with more familiarity can shed some light on what's expected here.

[jaredallard]

@dcormier Looks like this was introduced in 67337af. However, that was 7 years ago.. oof.

Maybe @johto has some ideas. To me it looks like it's expected to fail, and that test verifies that it fails so I'm unsure the issue going on here. Actually looking more into this, it is supposed to fail, but it errors when it does fail. This looks like a bad test to me and needs some help.

[13rac1]

Adding additional background, so others can find this PR more easily in a search.

This change is needed to avoid the default stdlib dsnConnector which throws out the Context.

func (t dsnConnector) Connect(_ context.Context) (driver.Conn, error) {
	return t.driver.Open(t.dsn)
}

https://github.com/golang/go/blob/8f4151ea67e1d498e0880f28d3fd803dc2c5448f/src/database/sql/sql.go#L701-L708

The TCP connection Dial() call blocks over two minutes on my machine (I assume this is OS specific) when given an inaccessible IP with or without Context.WithTimeout(). The workaround is to set connect_timeout=5 in the connection string. This works, but makes a new Context:

pq/conn.go

Lines 350 to 361 in 07d3f72

	// connect_timeout should apply to the entire connection establishment
	// procedure, so we both use a timeout for the TCP connection
	// establishment and set a deadline for doing the initial handshake.
	// The deadline is then reset after startup() is done.
	deadline := time.Now().Add(duration)
	var conn net.Conn
	if dctx, ok := d.(DialerContext); ok {
	ctx, cancel := context.WithTimeout(ctx, duration)
	defer cancel()
	conn, err = dctx.DialContext(ctx, network, address)
	} else {
	conn, err = d.DialTimeout(network, address, duration)

[13rac1]

@dcormier

The question is: why didn't it fail before?

The test setup is using sql.Open which uses the stdlib dsnConnector instead of the pq.Connector when DriverContext is not implemented:

pq/conn_test.go

Lines 50 to 51 in f91d341

	func openTestConnConninfo(conninfo string) (*sql.DB, error) {
	return sql.Open("postgres", testConninfo(conninfo))

database/sql/sql.go:Open() https://github.com/golang/go/blob/8f4151ea67e1d498e0880f28d3fd803dc2c5448f/src/database/sql/sql.go#L770-L778

	if driverCtx, ok := driveri.(driver.DriverContext); ok {
		connector, err := driverCtx.OpenConnector(dataSourceName)
		if err != nil {
			return nil, err
		}
		return OpenDB(connector), nil
	}

	return OpenDB(dsnConnector{dsn: dataSourceName, driver: driveri}), nil

When DriverContext is implemented then driverCtx.OpenConnector is called which runs pq.NewConnector during initialization generating the error client_encoding must be absent or 'UTF8'.

pq/connector.go

Lines 85 to 88 in f91d341

	if enc, ok := o["client_encoding"]; ok && !isUTF8(enc) {
	return nil, errors.New("client_encoding must be absent or 'UTF8'")
	}
	o["client_encoding"] = "UTF8"

Possible solutions:

1. Move this test to the NewConnector specific tests in connector_test.go
2. Refactor the test failure handling to check the NewConnector error returns earlier at

   pq/conn_test.go

   Lines 1485 to 1488 in f91d341

   	db, err := openTestConnConninfo(test.conninfo)
   	if err != nil {
   	t.Fatal(err)
   	}