Change https://golang.org/cl/52370 mentions this issue: database/sql: clean up Tx when panicking

@carl-mastrangelo Thank you for the report. I apologize for the delay in getting to this issue.

I'd like to back up a bit. Why do you consider the current behavior a bug? Or more exactly, can you describe how this behavior would be triggered outside of manually adding a panic?

@kardianos in the event of a panic, the connection is not properly released. I manually added one, but they can occur naturally. Better to program defensively.

I'm willing to listen to another opinion on this subject.

Unless I can see how a panic can happen in this situation I'm inclined to close this issue.

@kardianos what? The code is already written. It brings the sql code in the style of the rest of Go, and fixes the possibility of a panic. What's your hesitation? Perhaps someone else can review it?

I'm not seeing an error to fix here.
I'm not seeing the "defense" her as something needing to be defended against.

The proposed CL adds another closure which is another allocation and another function call. I would also argue it is slightly more complicated then the original code (although I like the switch statement).

Let me explain my reasoning. The following is common Go code:

f, err := os.Open("myfile.txt")
if err != nil {
    return err
}
_, err = io.Copy(os.Stdout, f) // This will not panic.
f.Close() // So this is safe.
if err != nil {
   return err
}

Or this example:

mutex.Lock()
sharedMap["foo"] = 42 // This will not panic.
mutex.Unlock() // So this is safe.

Yes, we commonly use defer to ensure resources are closed. However a deeper style/convention is to ensure functions don't panic in the first place. Go makes this easy to check and ensure. Panics don't just "happen". One way the sql package ensures locks are properly used is to use defer when unlocking locks around driver (user package) code. So in principle I agree with you. But defer is cheep and not free. Allocation is cheep, but not free. I have no issue making an allocation when needed, but unlike other languages panics / exceptions are tightly controlled.

On another note, you are not responding to my questions, but just saying "Why? Just merge it already, it is already there and it is better." This is not a productive way forward in any conversation. I've already told you my hesitation. I've now expanded on my hesitation. You are free to refer to someone else to this issue, but I would prefer to first direct your attention and reply to my questions and statements. If we discover they are false or without merit I will happily reverse my decision.

I'm not seeing an error to fix here.

There is no error, just a sharp edge. That is what needs to be fixed.

I'm not seeing the "defense" her as something needing to be defended against.

Moving cleanup code as close to the creation point is good style, in any language.

However a deeper style/convention is to ensure functions don't panic in the first place.

I think you are intentionally missing the intent of this change. Unless you can statically prove there are no panics possible, why take the chance? Furthermore, it is closer stylistically to the rest of the sql package. Why be a cowboy?

The proposed CL adds another closure which is another allocation and another function call.

That is the worst possible reason. If you look at my other contributions to the Go project, they have all been performance related. The change as written will not have a measurable impact.

On another note, you are not responding to my questions, but just saying "Why? Just merge it already, it is already there and it is better."

Because you don't see the problem you act on behalf of the whole project. Did you look at the code at all? Even if it didn't match up with anything I said prior, you can see the code is structured better:

• The retry logic is separate from the execution logic.
• The code is now resilient to unforseen panics.
• Even if panics are provably impossible today, code changes; they could be possible in the future.
• The maxBadConnRetries + 1 wart is gone.

You are free to refer to someone else to this issue

I would actually. Who is the next most appropriate reviewer for the database/sql package?