Add functions to check if an error is a given pgconn error

[ajruckman]

I have a need to check whether an error is a pgconn.writeError or pgconn.connLockError for a project, but these errors are not exported.

[jackc]

I have two concerns with this.

Types errors, whether directly exposed as public types or through Is* methods as you have done here, become part of the public interface of the package. And it is a public interface that requires extra care to avoid breaking. e.g. It's unlikely to accidentally make a breaking change to a functions arguments, but it is easy to accidentally change the return type of error -- especially when the error could originate several calls below where it is returned to the external code. In fact, while reviewing this PR and looking through the existing code I found and fixed an error where the wrong error type was being returned (ac364e7a4366fc363b67cbfc06edf41594d9d8cc). Though it is a bug fix, it also would technically be a breaking change if error types were exposed.

It only makes sense to expose error type information if it can be meaningfully acted upon by the caller. Rather than exposing all error types, every exposure needs to be justified by something the caller would do differently. For example, the only time the proposed IsPgConnError() would be true is when calling ReceiveMessage. I do not see what the application code could do with that information. By contrast, with SafeToRetry() can decide whether to automatically retry a failed query.

Secondly, even if we decide to expose some additional type information I don't think that IsErrorType() is the correct abstraction. If we really want to expose the exact type of the error then the error type should be made public and a normal type assertion or errors.Is should be used. Where functions make sense is when multiple error types may satisfy a particular condition. e.g. Multiple errors are true for Timeout() and SafeToRetry().

So what we need to know to go forward is what decision does the calling code want to make and then what is the smallest interface in pgconn that will reveal sufficient information to the calling code to make that decision.

[ajruckman]

I agree that the error.Is method is better than having multiple IsError() functions. I considered simply exporting the error types. Because all of their fields are unexported anyways this wouldn't expose any information that doesn't need to be exposed, and that method also requires less maintenance. But I decided against this because I wanted to modify as little code as possible.

My problem may be better solved with new method like SafeToRetry() as you said. I would like to check whether pgx.Conn.Ping() and other functions return an error indicating that the Postgres server has gone offline. After the server has gone offline, the first Ping() returns a *pgconn.writeError, but after that it always returns a *pgconn.connLockError.

I could create a function Offline() to check if an error indicates the server has gone offline. The *pgconn.connLockErrors always have status 'conn closed' so that is easy to check for. The *pgconn.writeErrors wrap a *net.OpError which wrap a *os.SyscallError which equals the value syscall.ECONNRESET.Error(). If you like this idea I can update this PR with a function that checks an error like this.

[jackc]

This is much better. I think we are getting really close to an ideal solution. But I have two more small points to bring up.

First, in general it is safe to consider any failure from pgx.Conn.Ping() to mean the connection was offline. If you only want to check the results of ping, no changes are needed.

But assuming this is something that could or should be used on an error returned from any function then I'd like to consider other names. Offline() has a couple ambiguities in my mind.

1. Does it mean the server is offline or our connection is offline?
2. Is this error permanent? i.e. a reasonable assumption might be to try again and maybe it will be online. But in pgx a broken network connection will not automatically be reestablished.

I would propose Unrecoverable(). This clearly states that the connection is no longer usable and will never be. On the other hand, if you are directly interested in if the error was a network failure this function does not reveal that. (However, errors.Is should be usable in that case to get the underlying net.Error.