Don't panic and errcheck

[ainar-g]

I've noticed that the code uses panic for error handling in several places. In this issue I propose removing those panics and instead use explicit errors everywhere. To ensure that all errors are checked or explicitly ignored, I propose adding the errcheck tool to the TravisCI script.

Any reasons not to do this?

[maddyblue]

Some of us have wanted to do this for a while but lacked motivation. Please go ahead, I'd love to see this change.

[chrispassas]

I agree, this would be a welcome change.

[ainar-g]

@mjibson What should I do about "impossible" panics? Like panic("unreachable") and those that marked as "this shouldn't happen"? In my code I usually don't panic at all, so "impossible" code paths return an error, but I'm not sure if we want that in lib/pq as well.

[maddyblue]

I think they should return an error.

[maddyblue]

Let me revise that. I'm looking at ListenerConn.Ping. I think it should be:

func (l *ListenerConn) Ping() error {
	_, err := l.ExecSimpleQuery("")
	return err
}

Instead of all the complication it has now. Are there other examples?

[ainar-g]

Sanity checks in (*ListenerConn).ExecSimpleQuery, (*Listener).disconnectCleanup(), hQuote, etc.

[maddyblue]

hQuote is fine as is. Other go functions in the stdlib panic on bad inputs. Unsure about the other two. I looked at them and it's not immediately clear if you can just return an error. It feels like you also need to make it do the other side effects of the err recovery catcher, which set the conn to bad. I don't have the time to dig deeper now.

Overall guidance about removing panics is: make sure the side effects from any defers that now won't happen are the same.

[ainar-g]

@mjibson If we plan on adding errcheck to the CI, should I also explicitly ignore errors from (*conn).Close() and net.Conn.Close()? Most of the time something is closed when there is already an error, so we either ignore the closing error with

// We don't care about errors from Close.
_ = cn.Close()

, or do something like

return fmt.Errorf("main err: %q, close err: %q", err, errClose)