Should `net.OpError` not result in an `ErrBadConn`?

[jenanwise]

Hi there!

In errRecover, net.OpErrors are turned into driver.ErrBadConn. My understanding is that drivers should never return driver.ErrBadConn if work may been completed on the server. The net.OpError handling was added with b48e1a2, but I don't see an explanation for the original intent of the current behavior. I assume it was for handling initial connection errors, but isn't it reasonable that a net.OpError may be triggered on an existing connection, potentially in the middle of a query?

My particular use case is: I've implemented a custom driver that wraps pq and sets a connection read and write deadline on every Read() and Write(). If the timeout hits, it will return a net.OpError. In this case, the connection should be marked bad, but the query not retried.

Am I missing something about the way pq handles this error?

[johto]

Am I missing something about the way pq handles this error?

Probably the fact that pq implements ErrBadConn incorrectly in this regard. I've previously stated that this should be changed, but that school of thought didn't gain much support.

[jenanwise]

Aren't you the maintainer? 😁

Changing

    case *net.OpError:
        *err = driver.ErrBadConn

to

    case *net.OpError:
        c.bad = true
        *err = v

doesn't seem to affect the test suite... probably because that particular path needs some tests. I'd be happy to submit a patch if I'm on the right path.

[johto]

Aren't you the maintainer?

I am not and never have been "the maintainer". I'm one of the maintainers, thought I was probably the most active maintainer a while ago. But I can't decide things like this on behalf of the project.

In any case, I think I've made my position clear. Other maintainers are free to chime in.

[msakrejda]

I merge straightforward patches here and there but I'm not actively using Go these days, so my opinion shouldn't hold much weight here. @johto do you mean #219? For what it's worth, I think the change is reasonable. Any thoughts on this @freeformz?

[freeformz]

To quote the docs:

To prevent duplicate operations, ErrBadConn should NOT be returned if there's a possibility that the database server might have performed the operation. Even if the server sends back an error, you shouldn't return ErrBadConn.

We can never assume a net.OpError doesn't violate that since it encompasses so many different errors. Generally speaking I agree with @johto, at least at first glance.

[carbocation]

I'm also running into driver.ErrBadConn in places where I don't expect it. It would be clarifying if the above changes were made, which would help narrow down whether an ErrBadConn was being appropriately (per the driver docs) emitted or not.

[andreimatei]

Hey lib/pq friends, this bug is real and it's a biggie. Go retries queries that it shouldn't because of the blunt translation of net.OpError to driver.ErrBadConn and drains unsuspecting bank accounts around the world. I've seen it with my own eyes.

cc my buddy @mjibson