Seems like there's improper error handling in pgconn.ConnectConfig

The condition in the following line

https://github.com/jackc/pgconn/blob/v1.5.0/pgconn.go#L142

should be inverse (!ok), otherwise it's impossible to access PgError to programmatically analyze cause of connection failure. Furthermore, current condition contradicts comment in line

https://github.com/jackc/pgconn/blob/v1.5.0/pgconn.go#L148

please update to a later version and publish up the chain to pgx

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-14040

Somewhere between 1.5.1 and 1.6.0, I'm seeing a pretty significant performance loss, mostly in memory allocations. Before, I was on v1.5.1-0.20200408003821-5d2be99c254e, which on one test would give me:

goos: linux
goarch: amd64
pkg: github.com/hortbot/hortbot/internal/bot
BenchmarkHandleNopParallel-4   	    4662	    252455 ns/op	    7962 B/op	      92 allocs/op
PASS
ok  	github.com/hortbot/hortbot/internal/bot	4.846s

But after upgrading, I get:

goos: linux
goarch: amd64
pkg: github.com/hortbot/hortbot/internal/bot
BenchmarkHandleNopParallel-4   	    4244	    266762 ns/op	   15069 B/op	     259 allocs/op
PASS
ok  	github.com/hortbot/hortbot/internal/bot	4.711s

Bisecting is a bit difficult because PRs here aren't squashed merged, but attempting to do it by hand shows that #36 was the point at which the performance changed.

Before, my pprof (allocation count) would look like:

But after, a lot of the memory allocations are performed elsewhere:

I've tested this in my project (https://github.com/hortbot/hortbot) by comparing the latest pgconn with what I get after doing go get -d github.com/jackc/pgconn@5d2be99c254e.

(I realize this could be more to do with pgx than pgconn, but I filed it here given downgrading pgconn only showed a difference.)

I'm curious if others have used the jackc/pgx/pgconn package with the CloudSQL proxy.

I have a type that implements the pgtype.BinaryDecoder and pgtype.BinaryEncoder interfaces, and things work well when using pgx.Connect and registering my type with RegisterDataType, but I'm uncertain if I would be able to do the same using the "cloudsqlpostgres" driver.

When trying to establish a connection using MD5 auth method, if authentication fails due to wrong password pgconn will skip checking fallback configs. However if authentication method used when connecting is SCRAM-SHA-256 then even after failing with wrong password it will continue checking fallback configs.
Digging deeper into the code the problem seems to lay in the following code lines in pgconn.go:148:

for _, fc := range fallbackConfigs {
		pgConn, err = connect(ctx, config, fc)
		if err == nil {
			break
		} else if err, ok := err.(*PgError); ok {
			return nil, &connectError{config: config, msg: "server error", err: err}
		}
	}

Namely, connect function call will return PgError when MD5 auth fails, but will return connectError that wraps PgError for SCRAM-SHA-256. So basically condition else if err, ok := err.(*PgError); ok will not be hit.
I assume that one of the workarounds is to inspect the root cause error of connectError and return if it's PgError but not sure how that plays with other fallback scenarios.

Within parseConfigError, it attempts to redact any user password. Unfortunately, if url.Parse fails for any reason, it simple proceeds with the DSN parsing logic, which causes a password to be leaked if it is part of the connection string.

I hit this by fuzz-testing some input on connection strings. In my case, I set port as "abc" and then got back the full DSN (including the password) back in the error message.

My hunch is if the string begins with postgres:// or postgresql:// and the URL cannot be parsed, it would be more safe to be cautious and not return the original connection string in the error message. It might be valuable to have redactPW return an error and perhaps adjust the behavior accordingly.

Hi!

When someone is using a connection pooler like Odyssey or PgBouncer they might want to enable pool_timeout (Odyssey) or query_wait_timeout(Bouncer) feature. In this case, the client waiting for some time for a free server connection might get ErrorResponse from the pooler followed by client disconnection.
Odyssey user will get "read tcp [2a02:6b8:0:2807:70d5:dc6a:5f2c:3882]:50336->[2a02:6b8:c1e:222f:0:1589:7844:129b]:6432: read: connection reset by peer". PgBouncer users will get simply "Unexpected EOF".

I think pooler has no change to pretend that Be\Fe sync persists. If we can fix this on the pooler side I'd be happy to do so.

Can we make receiveMessage() return the first ErrorResponse it encounters even if MRR is not closed yet, but the connection is broken?

Many thanks for maintaining such useful lib!

I was trying to find how to start a transaction, from this document I should be doing conn.Begin but pgconn doesn't have a Begin?

Given a DSN of host= user= password= port= database, pgconn will throw a panic:

panic: runtime error: index out of range [0] with length 0 goroutine 1 [running]: 

github.com/jackc/pgconn.parseDSNSettings(0xc0000a23dc, 0x9, 0xd, 0x0, 0x0) 
/go/pkg/mod/github.com/jackc/[email protected]/config.go:500 +0x6ef 

github.com/jackc/pgconn.ParseConfig(0xc0000a23c0, 0x25, 0x20300000000000, 0x3e97331fffff, 0x0) 
/go/pkg/mod/github.com/jackc/[email protected]/config.go:205 +0x205e 

github.com/jackc/pgx/v4.ParseConfig(0xc0000a23c0, 0x25, 0xc0000a23c0, 0x25, 0x18eec60) /go/pkg/mod/github.com/jackc/pgx/[email protected]/conn.go:135 +0x4d

github.com/jackc/pgx/v4/stdlib.(*driverConnector).Connect(0xc00009e5e0, 0x147b840, 0xc0000a6010, 0xb8, 0x203000, 0x203000, 0x203000) /go/pkg/mod/github.com/jackc/pgx/[email protected]/stdlib/sql.go:218 +0x1f5

database/sql.(*DB).conn(0xc0001100c0, 0x147b840, 0xc0000a6010, 0xc0000f1601, 0x9a9af5, 0xb02ec0, 0xc000089710) /usr/local/go/src/database/sql/sql.go:1233 +0x201

database/sql.(*DB).PingContext(0xc0001100c0, 0x147b840, 0xc0000a6010, 0x25, 0xc000089710) /usr/local/go/src/database/sql/sql.go:801 +0x90

database/sql.(*DB).Ping(...) /usr/local/go/src/database/sql/sql.go:819

I would like to execute a query and have pgx exhaust&buffer the underlying cursor as eagerly as possible, while still providing me the familiar for res.Next() { res.Scan(...) } loop. This is necessary as processing of each result-row takes significant app-side time, making the entire resultset linger around longer than the pg session timeout.

Is there a straightforward way to do this, or do I need to implement the eager read myself, effectively duplicating the loop?

In the issue tracker there is a number of references to pgconn.MinReadBufferSize, but I do not see an obvious way to tweak it via pgxpool.ParseConfig() / pgxpool.ConnectConfig()

Context expiration is implemented by SetDeadline on the underlying net.Conn. This means that the error returned from a query can be the underlying net error instead of the context error. This is confusing for callers.

See jackc/pgx#831.

An attempt at fuzz testing found a panic caused by a faulty connection string input such as x=x\\ (backslash is escaped) which causes parseDSNSettings to panic. Because of this, attempts to call pgconn.Connect could cause unexpected crashes. The crash occurs here due to an attempt to create a slice with an ending beyond the underlying capacity.

Plz refer to casbin/gorm-adapter#115

When supplying the following DSN to ParseConfig():

host=localhost port=5433 user=postgres password= dbname=foo

the dbname option is ignored, and I get a connection to the default database (postgres) instead of the specified one.

If I change the order of the options like this:

dbname=foo host=localhost port=5433 user=postgres password=

I get the connection to the correct database.

I believe it may be caused by password= being empty, and breaking parsing of following options, but I'm not sure about this diagnosis.

I initially did it this way because I saw a timeout error in the general sense of "we ran out of time to do the thing" regardless of why we ran out of time. For example, when running in an HTTP handler and the browser stops the request which cancels the context.

However in jackc/pgx#831 @atombender and @adw1n both seemed to be against that. I am still kind of inclined to leave this alone, but if either of them or anyone else wants to make a case for that change I am open to it.

Postgresql recognizes dbname not database as the parameter for the database instance to connect to.

It would be interesting to have a Hijack method that took control of the underlying net.Conn from the PgConn. This would let pgconn be used to connect to the PostgreSQL server and have something else take over the actual connection.

The reciprocal function would be Construct which would take an already connected net.Conn and any other requirements and built a *PgConn.

Not sure of the actual utility but it seems like it would make pgconn much more composable with other projects.

Parse function (https://golang.org/pkg/net/url/#Parse) used in:

It's broken in net/url Parse f-tion:
https://golang.org/src/net/url/url.go?s=13280:13319#L473

I tried to use: https://golang.org/pkg/net/url/#ParseRequestURI
But it supports HTTP schema only. I think net/url package should have function to support different URI schemas. Could this be something for GO team?

This is an extreme corner case that is extremely unlikely to occur without purposely triggering it.

If the PostgreSQL server sends more data than can fit in the client's incoming network buffers and the client then sends more data than can fit in the server's incoming network buffer a deadlock will occur. The exact amount depends on the platform. On MacOS Catalina 1MB in each direction is enough to deadlock when using TCP on localhost. Less is necessary when using a Unix socket.

The only known way to trigger this is for a connection to listen on a channel and not perform any action until the network buffers are full of unread notifications. Then it needs to send a huge query that overfills the network buffers the other direction.

There are two possible solutions:

1. Read before any writes. As there is no non-blocking read available (golang/go#36973) it is unclear how much performance cost there would be or if it could be done reliably. (What is the magic time we wait for the read deadline? How short is too short?)
2. Use goroutines for network IO. The problems with this approach are performance (a proof of concept test had a 20+% penalty in one test), additional complexity, and backwards compatibility. On the plus side, this would remove the need for the special casing to avoid deadlocks in batch operations and the copy protocol. It could also make it easy to detect network interruptions or database shutdowns (jackc/pgx#672).

Both solutions have a large cost so it is unclear if this corner case is worth fixing.

func TestConnExecNotificationsFillReadBufferBeforeAttemptingWrite(t *testing.T) {
	t.Parallel()

	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
	defer cancel()

	pgConn, err := pgconn.Connect(ctx, os.Getenv("PGX_TEST_CONN_STRING"))
	require.NoError(t, err)
	defer closeConn(t, pgConn)

	_, err = pgConn.Exec(ctx, `listen a`).ReadAll()
	require.NoError(t, err)

	notifierConn, err := pgconn.Connect(ctx, os.Getenv("PGX_TEST_CONN_STRING"))
	require.NoError(t, err)
	defer closeConn(t, notifierConn)

	kilobyte := strings.Repeat("x", 1024)

	mrr := notifierConn.Exec(ctx, fmt.Sprintf(`select pg_notify('a', '%s' || n) from generate_series(1, %d) n`, kilobyte, 1000))
	err = mrr.Close()
	require.NoError(t, err)

	buf := &bytes.Buffer{}
	for i := 0; i < 1000; i++ {
		buf.WriteString(fmt.Sprintf("select '%s';\n", kilobyte))
	}

	_, err = pgConn.Exec(ctx, buf.String()).ReadAll()
	require.NoError(t, err, "apparent deadlock when PostgreSQL has sent too many messages before Exec")
}

Postgres has a CopyBoth protocol used in replication, which isn't exposed by this package (yet). I would like to contribute that, but I have some questions

• Would you accept such a contribution?
• What's a good way to test that? Postgres itself doesn't expose CopyBoth (as far as I can tell) except through the replication protocol, but relying on the replication protocol in tests does not feel like the best. I guess I can start up a replication connection, and a non replication connection and write into the non replication connection and check some bytes come down the wire?
• Does this function signature make sense to you?

// CopyBoth executes the copy command sql and copies all of r to the PostgreSQL server, writing results to w.
//
// Note: context cancellation will only interrupt operations on the underlying PostgreSQL network connection. Reads on r
// could still block.
func (pgConn *PgConn) CopyBoth(ctx context.Context, w io.Writer, r io.Reader, sql string) (CommandTag, error) {

I should be fine implementing this once I have an idea on how to actually test it, but that part seems tricky.

As a library, I think it's most likely to use ConnectConfig instead of Connect with url string. But ConnectConfig would check if it's parsed, so it need to call pgconn.ParseConfig("") first, which would include localhost:5432 as default fallback. It's seriously wrong IMO, because it would use wrong connection if the main connection failed to establish.
So, I'm wondering what's the original consideration? Could you please fix it?

Hello,

Was running into an issue in which CopyTo wasn't writing query results to the specified io.Writer. It looks like the type switch in pgconn.go line 1141 is expecting the data to come in as *pgproto3.CopyData, but I noticed the messages were coming in as *pgproto3.DataRow types.

		switch msg := msg.(type) {
		case *pgproto3.CopyDone:
		case *pgproto3.CopyData:
			_, err := w.Write(msg.Data)
			if err != nil {
				pgConn.asyncClose()
				return nil, err
			}
		case *pgproto3.ReadyForQuery:
			pgConn.unlock()
			return commandTag, pgErr
		case *pgproto3.CommandComplete:
			commandTag = CommandTag(msg.CommandTag)
		case *pgproto3.ErrorResponse:
			pgErr = ErrorResponseToPgError(msg)
		}

I'm currently running Postgres version 13.2 & tested this using a SELECT * query.

Ran across this in our CI tests.
Connect was hanging on TLS handshakes and failing tests after the test timeout.
Created a minimal unit test to reproduce like so:

func TestConnectTimeoutStuckOnTLSHandshake(t *testing.T) {
    ln, err := net.Listen("tcp", "127.0.0.1:")
    require.NoError(t, err)
    defer ln.Close()

    serverErrChan := make(chan error)
    defer close(serverErrChan)
    go func() {
        conn, err := ln.Accept()
        if err != nil {
            serverErrChan <- err
            return
        }
        defer conn.Close()

        var buf []byte
        _, err = conn.Read(buf)
        if err != nil {
            serverErrChan <- err
            return
        }

        // Sleeping to hang the TLS handshake.
        time.Sleep(time.Minute)
    }()

    parts := strings.Split(ln.Addr().String(), ":")
    host := parts[0]
    port := parts[1]
    connStr := fmt.Sprintf("host=%s port=%s", host, port)
    conf, err := pgconn.ParseConfig(connStr)
    require.NoError(t, err)
    conf.ConnectTimeout = time.Millisecond * 250

    errChan := make(chan error)
    defer close(errChan)
    go func() {
        _, err := pgconn.ConnectConfig(context.Background(), conf)
        errChan <- err
    }()

    select {
    case err = <-errChan:
        require.True(t, pgconn.Timeout(err), err)
    case err = <-serverErrChan:
        t.Fatalf("server failed with error: %s", err)
    case <-time.After(time.Millisecond * 500):
        t.Fatal("exceeded Connect timeout, expected timeout error to occur")
    }
}

Should be simple to fix by moving the contextWatcher to include TLS handshake.
I can do a PR if all this makes sense.

Thanks.

PG14 adds several new values for the target_session_attrs connection parameter. These obviously have use since libpq implements them, but in my use case, I was looking for a read-only setting so that I could have clients which only need to query the DB use the replica(s), and save hardware resources on the master for data ingestion.

Yes, PG14 isn't out yet, but since this is a client-side feature, doesn't require any new support from the server, and is backwards compatible, it could be implemented now.

The new values are:

• read-only
  session must not accept read-write transactions by default (the converse)

• primary
  server must not be in hot standby mode

• standby
  server must be in hot standby mode

• prefer-standby
  first try to find a standby server, but if none of the listed hosts is a standby server, try again in any mode

https://www.postgresql.org/docs/14/libpq-connect.html

libpq will try all hosts. pgconn should match that behavior. This will be a little interesting to implement as it needs to be performed at dial time not at parse config time.

Errors are part of the package interface. Review to consider if errors are being exposed that shouldn't be or if there are new types or interfaces that should be.

The current error structure uses golang.org/x/xerrors to use the proposed Go 1.13 error system. PgError is the only exposed error type. There are several sentinel error values. At the moment all error checking must be performed with Is or As because of possible wrapping.

golang/go#29934
golang/go#32405

As stated in the title the postgres connection string postgresql format is not supported. For reference please see this link

At the moment, AfterConnectFunc is used both to test the validity of a connection for high-available scenarios and to set up the connection.

Split these uses into two functions. This will avoid the foot-gun of ParseConfig setting AfterConnectFunc because of target_session_attrs and the user inadvertently overriding it with an AfterConnectFunc designed to setup the connection.

These two functions will also have different error semantics. Errors returned from ValidateConnectionFunc mean try the next possible server whereas errors returned from AfterConnectFunc should be fatal to the connection attempt.

When multiple hosts are specified, libpq allows them to have different passwords set via the .pgpass file. pgconn should match this behavior.

I would be willing to implement this but want to make sure it would be desired. I propose the addition of a BeforeConnect hook, like AfterConnect that would be called here

I have a similar motivation to jackc/pgx#819 -- in my case, I'm using cockroachDB and would love to be able to rotate the host order on each connection and not bother with haproxy. I would implement a simple version of BeforeConnect that alters the Host and Fallbacks to randomly reorder the hosts. We could include it in this repository as well if desired, like ValidateConnectTargetSessionAttrsReadWrite.

I could also imagine this being useful for users that want to distribute load across standbys, or want to use the hostname to prioritize connecting to a location closer to where the app is running, things like that.

When using pgconn.ParseConfig on a connection string that doesn't specify a hostname, such as postgresql://root@:5432/postgres, pgconn.ParseConfig produces a config with a host of /tmp.

This appears to be because the default host is not overwritten, since the parsed host is not stored if it is empty:

Not sure if this is an unintentional limitation, but these hostname-less connection strings worked in older versions of pgx and generally works when using the Go network libraries, so it would be nice to support it.

There are quite a few non-exported error types that are returned by this API, such as parseConfigError and connectError that are not possible to be detected with errors.As or errors.Is. And unfortunately, using Unwrap() on them removes some useful context to determining the failure mode as well.

Would it be possible to include these particular errors (and probably all errors returned by this library) as part of the public API? One possibility is to just export these particular types as-is, with another option being to coalesce them into a single type.

For some context, I work on an application that accepts user input for connection settings in order to interact with their database, so I'm doing my best to handle as many errors as possible to make sure we provide descriptive and actionable error messages. The more structured context we can extract from error values, the better, and unexported types make that tricky if not impossible.

how to pass an array as a one parameter?
select * from table where value in ($1)
$1=['val1', 'val2'.....]

While *nix systems have the .pgpass file in the user's home directory, on Windows the path to this file is %APPDATA%\postgresql\pgpass.conf (as noted in the documentation).

(Weirdly, this does not seem to apply to .pg_service.conf, which remains in the home directory on Windows. 🤷)

Go makes it very easy to conditional-compile files based on operating system using build tags and magic filenames, so I would probably move defaultSettings() (and, once making changes, also defaultHost()) into their own file defaults.go with // +build !windows at the top, then add another file defaults_windows.go with the Windows versions of these functions.

I would be happy to submit a PR for this if you want.

I got this while I was debugging, where I forgot my context had a timeout and it was canceled as I was stepping leading to a rollback (since I rollback on an error instead of committing). It seems like there's a ContextWatcher race on rollback.

panic: Watch already in progress

goroutine 22113 [running]:
github.com/jackc/pgconn/internal/ctxwatch.(*ContextWatcher).Watch(0xc000096740, 0x145f440, 0xc0001d0840)
	/home/jake/go/pkg/mod/github.com/jackc/[email protected]/internal/ctxwatch/context_watcher.go:33 +0xdc
github.com/jackc/pgconn.(*PgConn).Close(0xc000d1e2c0, 0x145f440, 0xc0001d0840, 0x0, 0x0)
	/home/jake/go/pkg/mod/github.com/jackc/[email protected]/pgconn.go:497 +0x136
github.com/jackc/pgx/v4.(*Conn).die(0xc000955560, 0x1447020, 0xc0001d0800)
	/home/jake/go/pkg/mod/github.com/jackc/pgx/[email protected]/conn.go:357 +0x103
github.com/jackc/pgx/v4.(*dbTx).Rollback(0xc0008897d0, 0x145f480, 0xc0001a2010, 0x0, 0x0)
	/home/jake/go/pkg/mod/github.com/jackc/pgx/[email protected]/tx.go:186 +0x24f
github.com/jackc/pgx/v4/stdlib.wrapTx.Rollback(0x1467e00, 0xc0008897d0, 0x0, 0x0)
	/home/jake/go/pkg/mod/github.com/jackc/pgx/[email protected]/stdlib/sql.go:542 +0x5d
database/sql.(*Tx).rollback.func1()
	/usr/lib/go/src/database/sql/sql.go:2112 +0x4a
database/sql.withLock(0x144f6e0, 0xc000509e00, 0xc00074b778)
	/usr/lib/go/src/database/sql/sql.go:3184 +0x77
database/sql.(*Tx).rollback(0xc000ab4c80, 0x1, 0x0, 0x0)
	/usr/lib/go/src/database/sql/sql.go:2111 +0xce
database/sql.(*Tx).awaitDone(0xc000ab4c80)
	/usr/lib/go/src/database/sql/sql.go:2008 +0x67
created by database/sql.(*DB).beginDC
	/usr/lib/go/src/database/sql/sql.go:1723 +0x34e
Process exiting with code: 0

Hi,

I use 'pgx' driver in one of my project, and one of the user has created an issue related to reading password from .pgpass is not working.

I tried to dig into the problem and found, that UNIX path specified in host parameter of connection settings, is not used for searching the password in pgpass. Instead of this the "localhost" string is used, hence this leads that password is not found.

Steps to reproduce (setting up hba is omitted here, it is supposed that psql is successfully connecting using the .pgpass):

1. Create .pgpass file

/var/run/postgresql/:5432:postgres:vasya:vasya123

2. Use the following code to test connection.

func Test_Example(t *testing.T) {
        // Make sure that passfile is parsed successfully
	passfile, err := pgpassfile.ReadPassfile("/home/lesovsky/.pgpass")
	assert.NoError(t, err)

	for _, e := range passfile.Entries {
		fmt.Println(e)
	}

	cfg, err := pgx.ParseConfig("host=/var/run/postgresql/ port=5432 user=vasya dbname=postgres")
	assert.NoError(t, err)

        // password is empty, but must be "vasya123".
	assert.Empty(t, cfg.Password)}

After ParseConfig has been done, the Password is empty because "localhost" string is used for searching the password.

It is not critical for me and I have a workaround (use localhost instead of paths in .pgpass), but could you explain reasons of this behavior.

Hello, I get trouble with receiving error with upsert queries. SQL scheme and code snippet bellow.
I expect to get an error at rows, err := db.Query(context.Background(), query) but err there is nil .
In db log I get error ERROR: ON CONFLICT DO UPDATE command cannot affect row a second time after run this snippet.

SQL scheme:

 CREATE TABLE table_name
 (
   id      serial,
   guid    uuid,
   fkey_id int,
   actual bool,
   subject varchar(8)
 );
 CREATE UNIQUE INDEX i_uindex ON table_name (guid, fkey_id, actual);

code snippet:

package main

import (
	"context"
	"log"

	"github.com/jackc/pgx/v4/pgxpool"
)

const dsn = "user=postgres password=secret host=127.0.0.1 port=45432 dbname=db sslmode=disable"

func main() {
	c, err := pgxpool.ParseConfig(dsn)
	if err != nil {
		log.Panicf("%#v", err)
	}

	db, err := pgxpool.ConnectConfig(context.Background(), c)
	if err != nil {
		log.Panicf("%#v", err)
	}

	query := `
INSERT INTO table_name (guid, fkey_id, actual, subject) VALUES
	('8b42f9c3-d8d1-42b7-9b9d-b89513218d98', 1, true, 'subj'),
	('8b42f9c3-d8d1-42b7-9b9d-b89513218d98', 1, true, 'subj')
ON CONFLICT (guid, fkey_id, actual) DO UPDATE SET
    subject = EXCLUDED.subject
RETURNING id`

	rows, err := db.Query(context.Background(), query)
	if err != nil {
		log.Panicf("%#v", err)
	}

	var id int
	for rows.Next() {
		if err := rows.Scan(&id); err != nil {
			log.Panicf("%#v", err)
		}
		log.Print(id)
	}
}

I think current error message lacks some important information.
For example, when I try to insert a record and operation violates some constraint, then error message (PgError.Message) does not contain info about which record caused the error. It only says than constraint violation occured.
Could you please add Detail filed to (pe *PgError) Error() method?
I know that I can cast err to pgconn.PgError, but I think it would be more conveniently to see error details by default.

I noticed a bug in this code:

The signalMessage() method starts a goroutine that closes the channel when a message is received. This makes the main loop call receiveMessage to process the message. However, receiveMessage ends up getting called on every subsequent iteration, because the channel is closed from then on.

The second call to receiveMessage gets stuck, because there was only 1 message to process.

On the other hand, if there's more than one message to receive, then CopyFrom can get stuck on Write(), because the database is waiting to transmit the message (ClientWrite) and not reading more data. The current code assumes that each Write can only trigger at most 1 notice in response.

Here's my crappy attempt at a fix:

diff --git a/pgconn.go b/pgconn.go
index e3f3aaf..69ff37b 100644
--- a/pgconn.go
+++ b/pgconn.go
@@ -997,35 +997,47 @@ func (pgConn *PgConn) CopyFrom(ctx context.Context, r io.Reader, sql string) (Co
 	buf = make([]byte, 0, 65536)
 	buf = append(buf, 'd')
 	sp := len(buf)
-	var readErr error
-	signalMessageChan := pgConn.signalMessage()
-	for readErr == nil && pgErr == nil {
-		var n int
-		n, readErr = r.Read(buf[5:cap(buf)])
-		if n > 0 {
-			buf = buf[0 : n+5]
-			pgio.SetInt32(buf[sp:], int32(n+4))
+	var (
+		readErr error
+		signalMessageChan = pgConn.signalMessage()
+		copyChan = make(chan error)
+	)
+	go func() {
+		for {
+			n, err := r.Read(buf[5:cap(buf)])
+			if n > 0 {
+				buf = buf[0 : n+5]
+				pgio.SetInt32(buf[sp:], int32(n+4))
 
-			_, err = pgConn.conn.Write(buf)
+				_, err = pgConn.conn.Write(buf)
+				if err != nil {
+					pgConn.hardClose()
+					copyChan <- err
+					break
+				}
+			}
 			if err != nil {
-				pgConn.hardClose()
-				return nil, err
+				copyChan <- err
+				break
 			}
 		}
-
+	}()
+	for readErr == nil && pgErr == nil {
 		select {
+		case err := <-copyChan:
+			readErr = err
 		case <-signalMessageChan:
 			msg, err := pgConn.receiveMessage()
 			if err != nil {
 				pgConn.hardClose()
 				return nil, err
 			}
+			signalMessageChan = pgConn.signalMessage()
 
 			switch msg := msg.(type) {
 			case *pgproto3.ErrorResponse:
 				pgErr = ErrorResponseToPgError(msg)
 			}
-		default:
 		}
 	}

hardClose() ought to be moved out of the goroutine, but I didn't bother doing that.

Can you confirm whether this is a bug? Thanks!

We triggered an issue while testing a pgxpool :

we tested shutting down the pgsql server while a long running query ( WaitForNotification() )

There is an issue with the instruction pgConn.status = connStatusClosed in file pgconn.go on line 488 :

(branch master on 2021-03-04) :

the status of the connection is switched to connStatusClosed, but the cleanupDone channel is not.

The status change turn all calls to PgConn.Close() and PgConn.asyncClose() into a noop, and no action can close the cleanupDone channel afterwards,

At the layers of pgxpool.Pool and puddle.Pool, this makes the resource non collectable, and pool.Close() turn into a 15 second blocking operation.

I committed a minimal example in the linked Pull Request, in the test function TestFrontendFatalErrExec (link here); in that test, the "FATAL" error is simulated by injecting an ErrorResponse through the frontend :

func TestFrontendFatalErrExec(t *testing.T) {
	config, err := pgconn.ParseConfig(os.Getenv("PGX_TEST_CONN_STRING"))
	require.NoError(t, err)

	var front *frontendWrapper
	config.BuildFrontend = func(r io.Reader, w io.Writer) pgconn.Frontend {
                // use BuildFrontend to inject a postgre frontend, in which we can inject any pg message
		wrapped := buildFrontend(r, w)
		front = &frontendWrapper{wrapped, nil}
		return front
	}

	conn, err := pgconn.ConnectConfig(context.Background(), config)
	require.NoError(t, err)

	// set frontend to return a "FATAL" message on next call
	front.msg = &pgproto3.ErrorResponse{Severity: "FATAL", Message: "unit testing fatal error"}

	_, err = conn.Exec(context.Background(), "SELECT 1").ReadAll()
	assert.Error(t, err)

	err = conn.Close(context.Background())
	assert.NoError(t, err)

        // the error is here :
        // after a fatal error, even if we explicitly call `.Close()` on the pgConn object, 
        // its 'CleanupDone()' channel isn't closed :
	select {
	case <-conn.CleanupDone():
		t.Log("ok, CleanupDone() is not blocking")

	default:
		assert.Fail(t, "connection closed but CleanupDone() still blocking")
	}
}

I think a possible fix is to add :

pgConn.conn.Close()
close(pgConn.cleanupDone)

right after line 489, but I am not sure about the implications (can cleanupDone be closed from some other place ?)

I think there is a secondary bug here, but at least the first issue is:

The default: statement on a select means that it will not block (instead of the behaviour on blocking until either of the cases are satisfied). I can't see any reason for this since the loop doesn't do anything else, and this means currently while waiting it is doing nothing but spinning at full CPU. This leaves a profile looking like:

+   43.85%  badpg   badpg-test        [.] runtime.selectgo                                                                                                                                
+   11.32%  badpg   badpg-test        [.] runtime.lock                                                                                                                                    
+   10.44%  badpg   badpg-test        [.] runtime.unlock                                                                                                                                  
+    5.69%  badpg   badpg-test        [.] runtime.selectrecv                                                                                                                              
+    5.40%  badpg   badpg-test        [.] runtime.sellock                                                                                                                                 
+    5.06%  badpg   badpg-test        [.] runtime.selunlock      

I don't see any reason not to remove the default clause. The second bug is probably pg not noticing a connection termination somewhere, but I'm not sure yet where.

In https://github.com/jackc/pgconn/blob/master/pgconn.go#L227 Watch() is called. Then a few lines futher down (https://github.com/jackc/pgconn/blob/master/pgconn.go#L291) the ValidateConnectFunc is called. If you pass target_session_attrs=read-write to your connection string, this will lead for https://github.com/jackc/pgconn/blob/master/config.go#L707 to be executed, which in turn calls ExecParams, execExtendedPrefix and then Watch() again. If you are passing a non Background() context to Connect(), this will fail. See stacktrace:

goroutine 1 [running]:
github.com/jackc/pgconn/internal/ctxwatch.(*ContextWatcher).Watch(0xc000db45a0, 0x148e560, 0xc000db2240)
	/go/pkg/mod/github.com/jackc/[email protected]/internal/ctxwatch/context_watcher.go:33 +0xb7
github.com/jackc/pgconn.(*PgConn).execExtendedPrefix(0xc000ea7760, 0x148e560, 0xc000db2240, 0x0, 0x0, 0x0, 0xc00123f798)
	/go/pkg/mod/github.com/jackc/[email protected]/pgconn.go:957 +0x3af
github.com/jackc/pgconn.(*PgConn).ExecParams(0xc000ea7760, 0x148e560, 0xc000db2240, 0x12bc165, 0x1a, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/go/pkg/mod/github.com/jackc/[email protected]/pgconn.go:888 +0x81
github.com/jackc/pgconn.ValidateConnectTargetSessionAttrsReadWrite(0x148e560, 0xc000db2240, 0xc000ea7760, 0x0, 0x0)
	/go/pkg/mod/github.com/jackc/[email protected]/config.go:682 +0xaa
github.com/jackc/pgconn.connect(0x148e560, 0xc000db2240, 0xc000de00e0, 0xc001263f00, 0x0, 0x0, 0x0)
	/go/pkg/mod/github.com/jackc/[email protected]/pgconn.go:285 +0x8ad
github.com/jackc/pgconn.ConnectConfig(0x148e560, 0xc000db2240, 0xc000de00e0, 0x1dbf400, 0x40eb00, 0xc000db4160)
	/go/pkg/mod/github.com/jackc/[email protected]/pgconn.go:139 +0x224
github.com/jackc/pgx/v4.connect(0x148e560, 0xc000db2240, 0xc000de0000, 0x0, 0x0, 0x40f448)
	/go/pkg/mod/github.com/jackc/pgx/[email protected]/conn.go:198 +0x161
github.com/jackc/pgx/v4.ConnectConfig(...)
	/go/pkg/mod/github.com/jackc/pgx/[email protected]/conn.go:105
github.com/jackc/pgx/v4/stdlib.(*driverConnector).Connect(0xc000db4160, 0x148e560, 0xc000db2240, 0x148e560, 0xc000db2240, 0xc000da82f0, 0xc00082b860)
	/go/pkg/mod/github.com/jackc/pgx/[email protected]/stdlib/sql.go:177 +0xbb
github.com/jackc/pgx/v4/stdlib.(*Driver).Open(0xc00071f660, 0x7ffe29c35db7, 0xc1, 0x0, 0x0, 0x0, 0x0)
	/go/pkg/mod/github.com/jackc/pgx/[email protected]/stdlib/sql.go:136 +0xff
github.com/gchaincl/sqlhooks.(*Driver).Open(0xc00069a400, 0x7ffe29c35db7, 0xc1, 0x0, 0x0, 0x40eb00, 0xc000da0000)
	/go/pkg/mod/github.com/gchaincl/[email protected]/sqlhooks.go:47 +0x4e
database/sql.dsnConnector.Connect(...)
	/usr/local/go/src/database/sql/sql.go:688
database/sql.(*DB).conn(0xc0003560c0, 0x148e520, 0xc0000aa000, 0xc00082bb01, 0x9ac415, 0x1294180, 0xc000da6420)
	/usr/local/go/src/database/sql/sql.go:1228 +0x201
database/sql.(*DB).PingContext(0xc0003560c0, 0x148e520, 0xc0000aa000, 0xc1, 0xc000da6420)
	/usr/local/go/src/database/sql/sql.go:782 +0x90
database/sql.(*DB).Ping(...)
	/usr/local/go/src/database/sql/sql.go:800```

Hi,

After updating library from 1.4.0 to 1.8.1 I have some issues with connections via unix socket.
I'm using connection string that parsed via pgx.ParseConfig
Connection string of style
postgres://user:pass@host:port/dbname?statement_cache_mode=describe&application_name=appname

In case I wouldn't provide host in it and leave it of style:
postgres://user:pass@:port/dbname?statement_cache_mode=describe&application_name=appname
Issue will be reproduced.

In version 1.4.0 it was working great. In case when host was not provided, it will connect via unix socket.
I studied code that processes connection string in both versions and found difference in treatment of empty host after parse.

In version 1.4.0 there were check:
if host==""{ continue }

something like that.

In 1.8.1 there is no such check, and empty host will be appended to hosts array in all cases.
config.go file line 414 changed to
if h != "" { hosts = append(hosts, h) }

I debugged it locally and found if that check will be returned to it place, I could connect via unix socket without problem

Change from above helped to fix my issue. Can you please add such check in new version?

Thanks

Consider if IsAlive is the correct interface and if simply checking the internal status is the correct implementation.

Hey there! I'd like to discuss ways to access the error code inside Postgres errors in a way that's compatible across pg drivers (pgconn/pgx and lib/pq) or, better yet, across any drivers compatible with Go's sql package. I figured this is as good of a place to discuss it as any.
cc @mjibson as a lib/pq maintainer and @kardianos as a Go sql contributor.

Here's the rub: CockroachDB offers this tiny cockroach-go library which contains some transaction helpers specific to CockroachDB. CockroachDB is wire-compatible with Postgres. cockroach-go tries to support both lib/pq and pgx (now pgconn), and so it has some unfortunate code that looks inside errors to check their error code, looking for a CRDB-specific retriable one:
https://github.com/cockroachdb/cockroach-go/blob/master/crdb/tx.go#L141-L152

Since there's seemingly no common way to get to these code, our library depends on both lib/pq and pgx and checks the concrete types of errors against pq.Error and pgx.PgError and then uses the Code fields in these structs (which fields, luckily, were public).
Recently, pgx published a new version which changed the name of the error struct - it went from pgx.PgError to *pgconn.PgError. This broke our little library. I could upgrade it to depend on the new version of pgx (and accept that not all versions of cockroach-go work with all versions of pgx), but I'd like a better solution.
I'd like cockroach-go to not have to depend on either pgx or lib/pq. I'd like a common interface for these errors that I can test for - simply a PgCode() string method.
@jackc and @mjibson - would you mind adding such an interface to the respective errors?

Or, @kardianos, any hope for the sql package itself to help in my quest?

Thanks!

This could be used to configure the buffer size of the chunkreader.

I'm seeing races like this in my CI after upgrading pgconn:

2020-01-12T06:29:03.4238312Z WARNING: DATA RACE
2020-01-12T06:29:03.4238463Z Write at 0x00c00025ecd9 by goroutine 70:
2020-01-12T06:29:03.4238768Z   github.com/jackc/pgconn/internal/ctxwatch.(*ContextWatcher).Watch()
2020-01-12T06:29:03.4239018Z       /home/runner/go/pkg/mod/github.com/jackc/[email protected]/internal/ctxwatch/context_watcher.go:36 +0x63
2020-01-12T06:29:03.4239572Z   github.com/jackc/pgconn.(*PgConn).ayncClose.func1()
2020-01-12T06:29:03.4239784Z       /home/runner/go/pkg/mod/github.com/jackc/[email protected]/pgconn.go:526 +0x192
2020-01-12T06:29:03.4239968Z 
2020-01-12T06:29:03.4240321Z Previous read at 0x00c00025ecd9 by goroutine 10:
2020-01-12T06:29:03.4240978Z   github.com/jackc/pgconn/internal/ctxwatch.(*ContextWatcher).Unwatch()
2020-01-12T06:29:03.4241429Z       /home/runner/go/pkg/mod/github.com/jackc/[email protected]/internal/ctxwatch/context_watcher.go:59 +0x9c
2020-01-12T06:29:03.4241734Z   github.com/jackc/pgconn.connect()
2020-01-12T06:29:03.4241987Z       /home/runner/go/pkg/mod/github.com/jackc/[email protected]/pgconn.go:251 +0x2552
2020-01-12T06:29:03.4242340Z   github.com/jackc/pgconn.ConnectConfig()
2020-01-12T06:29:03.4242560Z       /home/runner/go/pkg/mod/github.com/jackc/[email protected]/pgconn.go:137 +0x388
2020-01-12T06:29:03.4242734Z   github.com/jackc/pgx/v4.connect()
2020-01-12T06:29:03.4242947Z       /home/runner/go/pkg/mod/github.com/jackc/pgx/[email protected]/conn.go:198 +0x35d
2020-01-12T06:29:03.4243270Z   github.com/jackc/pgx/v4/stdlib.(*Driver).Open()
2020-01-12T06:29:03.4243659Z       /home/runner/go/pkg/mod/github.com/jackc/pgx/[email protected]/conn.go:105 +0x19e
2020-01-12T06:29:03.4243838Z   database/sql.(*dsnConnector).Connect()
2020-01-12T06:29:03.4244385Z       /opt/hostedtoolcache/go/1.13.6/x64/src/database/sql/sql.go:688 +0x88
2020-01-12T06:29:03.4244583Z   database/sql.(*DB).conn()
2020-01-12T06:29:03.4244832Z       /opt/hostedtoolcache/go/1.13.6/x64/src/database/sql/sql.go:1228 +0xa15
2020-01-12T06:29:03.4245031Z   database/sql.(*DB).PingContext()
2020-01-12T06:29:03.4245548Z       /opt/hostedtoolcache/go/1.13.6/x64/src/database/sql/sql.go:782 +0x92
2020-01-12T06:29:03.4245824Z   github.com/hortbot/hortbot/internal/pkg/docker/dpostgres.newDB.func1()
2020-01-12T06:29:03.4246084Z       /opt/hostedtoolcache/go/1.13.6/x64/src/database/sql/sql.go:800 +0x245
2020-01-12T06:29:03.4246325Z   github.com/hortbot/hortbot/internal/pkg/docker.(*Container).Start.func2()
2020-01-12T06:29:03.4246585Z       /home/runner/work/hortbot/hortbot/internal/pkg/docker/docker.go:62 +0x62
2020-01-12T06:29:03.4246803Z   github.com/cenkalti/backoff/v3.RetryNotify()
2020-01-12T06:29:03.4247064Z       /home/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:37 +0xc8
2020-01-12T06:29:03.4247282Z   github.com/ory/dockertest/v3.(*Pool).Retry()
2020-01-12T06:29:03.4247746Z       /home/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:24 +0xeb
2020-01-12T06:29:03.4247960Z   github.com/hortbot/hortbot/internal/pkg/docker.(*Container).Start()
2020-01-12T06:29:03.4248180Z       /home/runner/work/hortbot/hortbot/internal/pkg/docker/docker.go:61 +0x3d9
2020-01-12T06:29:03.4248391Z   github.com/hortbot/hortbot/internal/pkg/docker/dpostgres.newDB()
2020-01-12T06:29:03.4248623Z       /home/runner/work/hortbot/hortbot/internal/pkg/docker/dpostgres/dpostgres.go:42 +0x2eb
2020-01-12T06:29:03.4248840Z   github.com/hortbot/hortbot/internal/db/migrations_test.withDatabase()
2020-01-12T06:29:03.4249236Z       /home/runner/work/hortbot/hortbot/internal/pkg/docker/dpostgres/dpostgres.go:20 +0x5c
2020-01-12T06:29:03.4249462Z   github.com/hortbot/hortbot/internal/db/migrations_test.TestUpDown()
2020-01-12T06:29:03.4249690Z       /home/runner/work/hortbot/hortbot/internal/db/migrations/migrations_test.go:42 +0x52
2020-01-12T06:29:03.4249847Z   testing.tRunner()
2020-01-12T06:29:03.4250046Z       /opt/hostedtoolcache/go/1.13.6/x64/src/testing/testing.go:909 +0x199
2020-01-12T06:29:03.4250120Z 

Full logs for the build are shown here (including all race variants): https://github.com/hortbot/hortbot/commit/ba3d209a21a2df70997cf4f5486aaffe15a41b59/checks?check_suite_id=396092918

Looking at ContextWatcher, a number of field accesses are racy. The struct probably needs to be mutex'd, or refactored to work differently (the way it works seems perhaps a little awkward to my eye, but I haven't personally tried to fix this yet).

Currently when using the PgConn.CopyTo function you must supply the full sql as a string

var file *os.File
...
cmd, err = conn.PgConn().CopyTo(server.Context(), file, "COPY ( select * from tablexzy ) TO STDOUT WITH (FORMAT CSV, FORCE_QUOTE *)")

When the COPY sql becomes very dynamic you are force to stringify the sql, ideally if CopyTo had the arguments parameter like pgx.Conn has it would make things a whole lot easier.

var file *os.File
args := []interface{}{
   userId,
   groudIds
}
//column_1 text column and column_2 being an text array column
sql := `select * from tablexyz where $1 = column1 $2 && column_2`
...
cmd, err = conn.PgConn().CopyTo(server.Context(), file, fmt.Sprintf("COPY ( %s ) TO STDOUT WITH (FORMAT CSV, FORCE_QUOTE *)", sql),args...)

TxStatus, Frontend, and Config are public. Perhaps they shouldn't be.

After some trial and error, this is what I've come up with but wanted to get your opinion. My initial naive implementation used a plain select and that locked the connection until all the results were read. Meaning, I couldn't execute other statements until the reader was closed. Use Exec to open a cursor, Query/QueryRow to fetch rows from the cursor, and then Exec to close the cursor, when done.

DECLARE dup_hash_crsr CURSOR FOR SELECT file_hash, count(*) AS image_count 
		FROM all_files 
		GROUP BY file_hash 
		HAVING count(*) > 1

• Execute FETCH 1 FROM dup_hash_crsr and read in all the rows from that fetch. Could be fetch 1, 10, 30 or whatever makes sense for the application.
• At this point the connection should be unlocked so I could execute other statements.
• Loop back to fetch.

Use CLOSE dup_hash_crsr to close the cursor.