Add PrepareConn hook, which extends BeforeAcquire's behavior to allow… #2329
Conversation
… canceling the instigating query - BeforeAcquire now marked as deprecated, and re-implemented in terms of PrepareConn - PrepareConn now takes precidence over BeforeAcquire if both are provided - New tests added, so both old and new behavior are tested - One niggle: AcquireAllIdle does not return an error, so the only recourse that seems reasonable when PrepareConn returns an error in that context, is to destroy the connection. This more or less retains the spirit of existing functionality, without changing the public API of that method. Although maybe an error-returning variant would be a useful addition as well.
There was a problem hiding this comment.
After looking at the comments describing the return values for PrepareConn, I'm now questioning its interface. It definitely needs to be read a few times to understand. It's tricky because we are using it to signal multiple things on failure.
- Should the connection be destroyed?
- Should the
PrepareConnbe retried on a new connection? - What error should be returned to the caller?
I'm thinking that whether an error is returned should be the sole determinant of whether PrepareConn succeeded or not and if Acquire ultimately fails that should be the error returned to the caller.
But that still leaves "should destroy" and "should retry".
I wonder if the following signature would be clearer:
func(context.Context, *pgx.Conn) (retry bool, destroyConn, err error)Or maybe we should create a specific PrepareConnError type or interface that has those fields or methods. That could allow for further extensibility. I don't see what extensibility would be needed, but obviously I didn't foresee the insufficiency of BeforeAcquire either.
Or maybe fixing / improving the comments further would be sufficient. 🤷
| // - It must return true and a nil error to allow acquisition and the query to proceed. | ||
| // - If it returns true and an error, the connection will be returned to the pool, and the instigating query will fail with the returned error. | ||
| // - If it returns false, and an error, the query will fail with the returned error, and the connection will be destroyed. | ||
| // - If it returns false and a nil error, the connection will be returned to the pool, and the instigating query will be retried on a new connection. |
There was a problem hiding this comment.
Is this comment correct? I thought (false, nil) should destroy the connection.
There was a problem hiding this comment.
You are correct. The comment is wrong.
There was a problem hiding this comment.
I've updated the GoDoc, and made it a bit more symmetrical for ease of understanding. Give it a look and see if it's now clear enough, or if a different API is still a better idea.
There was a problem hiding this comment.
The new docs are much clearer. I think that will work. Merged!
… canceling the instigating query
Fixes: #2328