12 ms·
Three bugs in the Go MySQL driver
- Traubenfuchs 6y agoOne consequence of reinventing the wheel (go) is going through the same problems others stumbled over years ago.
- zkirill 6y agoAs someone who is currently transitioning from Go to Java/Kotlin I agree with you to an extent. However, I was surprised that I wasn't able to find anything in the Java/Kotlin universe that is comparable to Go's web request context cancellation.
- diek 6y agoCompletableFuture is the closest concept in Java, to my knowledge. java.util.concurrent has a ton of great classes. If you need to do shared memory concurrency, it probably has something there to help you.
- The_rationalist 6y agohttps://kotlinlang.org/docs/reference/coroutines/cancellation-and-timeouts.html https://kotlinlang.org/docs/reference/coroutines/cancellatio...
- apta 6y agoLoom will have superior context cancellation and deadline handling than golang. https://cr.openjdk.java.net/~rpressler/loom/loom/sol1_part1.html https://cr.openjdk.java.net/~rpressler/loom/loom/sol1_part1....
- Traubenfuchs 6y agoI didn't read all of it, but I was wondering: In a virtual loom thread, am I allowed to call currentThread().sleep(...); or will it block the system thread? Or is there some magic like currentThread().sleepAndThen(..., ()->{}); ?
- bgrainger 6y agoWhile the second and third problems sound specific to Go's `database/sql` API, the first problem (server-closed connections) is an issue any MySQL client library (that implements connection pooling) has to deal with. My .NET MySQL connector (https://github.com/mysql-net/MySqlConnector https://github.com/mysql-net/MySqlConnector) "solves" it at the MySQL level by sending a PING packet, but as the post points out, this adds extra latency. The TCP-based approach of performing a non-blocking read sounds like a much better approach; I'm glad the author shared this, and I want to see if that technique can also be implemented in MySqlConnector: https://github.com/mysql-net/MySqlConnector/issues/821 https://github.com/mysql-net/MySqlConnector/issues/821.
- wahern 6y agoThe non-blocking read is a bad hack to an ill-defined problem. For starters, if a read would fail, so would a write. Your write path has to handle errors anyhow, so the non-blocking read is a contrivance. I understand the reason for the contrivance--it's because the typical SQL driver API requires first dequeueing a connection from a pool, passing ownership to a caller, and the caller sending a query. The proper and better API is to combine the "dequeue connection" and "send query" steps, returning a handle back for the response, which may include (as an optimization) assigning the connection to the caller for any additional requests. Additionally, any hack that prevents the majority of write failures, but not all, leaves time bombs around by not stressing the write failure paths of the caller. Secondly, IME a far more common reason in the wild for a connection disappearing is either a) the server crashing or b) NAT association timeout. In either case it's typical for the remote end to simply disappear, and unless TCP keepalive is enabled, a read won't return any error until you first attempt a write and wait for the TCP stack to figure out that the remote end is gone. In the best case that's a R/T, in the worst case some eager firewall rules in the middle or on the remote peer silently drop packets for unassociated connections and/or the ICMP response and you're stuck waiting for TCP timeout. Thirdly, and related to #2, absent TCP keepalive or an application ping (not ICMP ping), connections are far more likely to get dropped on the floor while sitting in the pool. And an application ping is more reliable because some firewalls and application and NAT gateways drop or ignore TCP keepalive. I've written client libraries with connection pooling for HTTP, MySQL, SMTP, RTSP and generic TCP. I learned all this the hard way.
- nerdbaggy 6y agoI’m sure there are a lot of reasons to hate it but I like how Go does the different database types and the base type
- nkozyra 6y agoA lot of languages do this under the hood, because sql abstraction layers need it for network, query discrepancies.
- jijji 6y agoI encountered this same issue a few years ago when using the Go mysql driver, and the simple fix was calling db.Ping() before doing a query, which checks the connection and allows the query to proceed.
- RhodesianHunter 6y agoSo you have to make a round trip to the database before making a query? Yikes.
- curryst 6y agoI think you could also accomplish the same thing with a goroutine. Just a struct that stores a RW mutex and a sql.DB connection that has a .Start() function that runs the ping on a loop. On each start of the loop, lock the write mutex, run the ping, reconnect if it fails, and then release the mutex. Then just wrap each of the functions in sql.DB so that it gets a read lock before it executes the sql.DB function and defer the release. It's still a performance penalty, but it moves the round trip to a 99% latency situation instead of an every query situation.
- kevingadd 6y agoYou don't need a round-trip, you can push a ping request into the pipe immediately followed by your actual query. You don't care about the actual result of the ping unless your query fails - "if the ping also failed, this was a dead connection so I can retry the query, but if the ping was OK and the query was not, the query failed"
- meritt 6y agoThe general approach I've used in other languages is if the mysql client API is implemented correctly [1], when it encounters a closed connection, will return a CR_SERVER_GONE_ERROR error to the calling code. You catch when this type of error occurs, re-initialize the connection, and replay the last query/transaction. [1] https://dev.mysql.com/doc/refman/8.0/en/gone-away.html https://dev.mysql.com/doc/refman/8.0/en/gone-away.html
- kardianos 6y agoFYI: Go 1.15 will have idle timeout: https://tip.golang.org/pkg/database/sql/#DB.SetConnMaxIdleTime https://tip.golang.org/pkg/database/sql/#DB.SetConnMaxIdleTi... The driver interface also separates out the session resetter and the connection validator concepts. https://tip.golang.org/pkg/database/sql/driver/ https://tip.golang.org/pkg/database/sql/driver/ Please do have drivers implement the Connector.
- sudhirj 6y agoWhy aren’t these problems applicable to Postgres? Is the connection model different or do the libraries already do all this?
- hilbertseries 6y agoWhy is Postgres relevant to this post at all?
- donatj 6y agoBecause Go uses the same provided abstraction layer for all SQL databases. If the errors are in the abstraction layer and not the driver it would be odd for them to be MySQL specific.
- wolco 6y agoNot really that odd. The abstraction layer provides functionality common to all. Perhaps something common didn't work for mysql.
- csharptwdec19 6y agoEntirely possible. If you've ever looked at an ORM or even complex MicroORM that supports multiple database you get an appreciation for how many 'little' differences everyone has in the SQL standard itself. And if that's the "industry standard" (Yeah, I know it's not, but it's at least a spec/guideline) with that much deviation, I can only imagine how dissimilar each DB's nonstandard protocol might be.
- infogulch 6y agoI could see GP's comment as a way of exploring the problem in order to get a better grasp on the technical or architectural choices that may have caused the difference in outcome, in order to potentially learn what's important about how to design things like connection pooling.
- sargun 6y agoThese bugs are not specific to MySQL. The bug around context closing the connection resulting in a txn not having an explicitly successful abort / rollback is a problem as well because conn.Close gets called in a goroutine when context expires. We've experienced this in pq as well.
- todotask 6y agoThis as well, a recently fix for this scenario https://github.com/golang/go/issues/34775 https://github.com/golang/go/issues/34775
- zkirill 6y ago>> Instead, make sure that every SQL operation in your app is using the QueryContext / ExecContext interfaces I've been wondering if this is indeed a best practice or something that should be used only when necessary. Should every database query really abort if the connection to the client fails? In addition, if you follow this advice you now need to check if the returned error is context.Cancelled which warrants an HTTP 400 response.
- dullgiulio 6y agoOf course it is up to the application to decide case-by-case. Just keep in mind that if your query is started by a user request, but does not need to terminate in the context of the user request that triggered it, you can move the query to a separate goroutine and decouple it completely from the triggering user request. Also, aborting a query is safe: the transaction gets rolled back, removing unwanted side effects. You cannot do harm by cancelling, bu you can do harm if you don't cancel.
- nitwit005 6y ago> A quick Code Search for rows.Close() on GitHub shows that pretty much nobody is explicitly checking the return value of rows.Close() in their Go code. Seems to be a pattern in all languages. Plenty of C code ignoring the result of the close system calls, or Java code that just wraps exceptions from close calls in try/catch blocks that do nothing with the exception.
- PeterisP 6y agoWell, what should the code do there? At that point, the status is uncertain, there's potentially a problem but IMHO there's nothing the code can do to reliably recover from it.
- sudhirj 6y agoThe code should throw away the data it read from the connection just before it closed it, because there's a high chance of it being corrupt.
- siscia 6y agoWhat if it is a write?
- sudhirj 6y agoThat complicates things, but no different from seeing an error either way. The point is that checking for an error at point one is not enough, if you want to be sure there wasn't an error you need to check point 2 as well. In the case of a write, if you seen the success message at point 1 you might not care about a failure at point 2, because you already know the write was a success, assuming your confirmation at point 1 was explicit and well formed. In the case of read, say you read an email address, and received 'someone@somwhere.co'. The thing is, if the connection was cut you don't know if that address was supposed to be '.com', '.co.in' or '.co'. The only way you can tell in the current MySQL protocol is to check for an error on point 2 (the Close()). If you see an error, you know the read was incomplete, and you throw it away. If you don't check, you might have read the wrong (corrupt) value.
- kccqzy 6y agoCancellation is always such a big issue regardless of language or environment, and whether it's due to a timeout or some other action. In C, when using threads and thread cancellation there are a lot of subtleties involved: https://ewontfix.com/2/ https://ewontfix.com/2/ Even in Haskell where there's generally a focus on well designed abstractions, asynchronous cancellation can fail due to badly written code that wants to catch all exceptions.
- erikrothoff 6y agoAs someone running MySQL with Go in production and seeing random MySQL connection loss exceptions, is there a summary of how they fixed this? It’s an insanely long article and I appreciate the detail and effort, but if someone has read it all and have a gist that would be insanely helpful .
- sethammons 6y agoYou have production issues related to the article but you dont have the time to read the article?
- erikrothoff 6y agoSorry if I came across as lazy. It’s a sporadic issue that’s more easily fixed with a service restart. Did not mean to come off as entitled. My intent was just to ask for some help from the community.
- earthboundkid 6y agoThe article has three headings for “production tips”. TL;DR read those.
- erikrothoff 6y agoThanks! I ended up reading the entire article, and didn’t really understand it all, but I think tl;dr upgrade to go mysql driver 1.5.
- yuribro 6y agoI really don't understand the discussion around the first bug. Either something is over simplified, or is it just an issue of a bad abstraction? This issue (other side closes connection) is so fundamental to all networking code, I don't see how the use specific use case (MySQL idle timeout) is special. The connection pool also doesn't sound relevant - the same would happen if the caller is keeping the same connection (socket) and using it. For this class of issues - this is the easiest case to deal with - the other side sent a FIN and we got it! Our kernel knows that this socket is closed (doesn't matter if fully closed or only half for this case). So if you would do a select (or equivalent) call for the socket it will not return as writable (and will probably return in the exception case), and you'll reconnect. If you don't use select - the write will fail immediately, and I would guess that at the worst case, you can see that 0 bytes were written? So the problem here was just that the wrong error was returned through the multiple layers of abstractions? Why not propagate the error correctly? Or handle the re connection in a lower level? Trying to "Ping" on the application level is such an overkill and misuse of resources. Do we really want to have an extra round trip for every action we do on a DB? Of course, as someone else pointed out, there are much harder issues to consider - silent disconnects, proxied connections where the proxy didn't propagate the error, failures on the DB level, and so on. EDIT: I see now that another comment mentions that there is a specific error code in MySQL clients for lost server.
- frobisher 6y agoJust curious - are any of the these bugs possible in Rust?
- The_rationalist 6y agoRust SQL drivers (when they exists) are mostly not production ready, they just have not the human resources. For example the leading driver (diesel) do not support elementary SQL such as GROUP BY
- couchand 6y agoI thought Diesel was just an ORM, but investigating your comment I discovered that they've implemented their own drivers in-tree. That surprises me, and also seems like a mistake. Steven Fackler's PostgreSQL driver, for instance, is widely considered to be quite solid.
- JulienSchmidt 6y agoBug #3 (The race) was silently introduced by a semantic change in Go's database/sql in January 2018: https://github.com/golang/go/commit/651ddbdb5056ded455f47f9c494c67b389622a47 https://github.com/golang/go/commit/651ddbdb5056ded455f47f9c... It took until December of the same year until we got the first bug report and figured out what was going on. While the semantic change might look subtle, it was certainly not from our (driver maintainer's) perspective. We were quite disappointed that such a change was made 1) without informing the driver maintainers 2) making sure the changes were in place before this change made it into a Go release. We regularly test against Go's master (now using a Travis CI cron job), but that only helps if the existing tests fail. We don't have the time to constantly monitor all changes in the Go repo. If there is a need to make such changes (not just in database/sql and not just in Go), PLEASE actively communicate early with the community / the direct users.
- nemetroid 6y agoA comment on the original issue[1] promises to ”[open] some tickets for various drivers”. Did this not happen? 1: https://github.com/golang/go/issues/23519#issuecomment-360572590 https://github.com/golang/go/issues/23519#issuecomment-36057...
- mfer 6y agoNotifying users of the code of a semantic change does not mean it's not a semantic change. Even if the Go team can find a way to notify all of the public drivers impacted, they aren't going to be able to know about the private drivers or forks to notify those people. Nor are they going to be able to notify all the consumers who, using MVS, have the version of the driver set to one that's not fixed. Bugs from this change will propagate and there's nothing the Go team can do to stop them all. This is why projects following semantic versioning are not supposed to make breaking changes in minor releases. This is, unfortunately, not the first time I've seen this kind of change in Go. Some previous changes have caused bugs for me a few times in the past. Not knowing which version of the compiler one would use I ended up needed to craft code to handle before and after cases for the change along with the bug reports from users.
- 6y ago
- The_rationalist 6y agoThe justification for going to go given here is very weak. Firstly if they really cared about performance they would go to jruby or truffleruby, secondly most of the performance of a language are in its library ecosystem beyond being the language itself. Ruby has had decades of optimisation for the server that the nascent go has to reinvent
- PunksATawnyFill 6y agoGo: Yet another Google language. What's it going to be next week?
- nkozyra 6y agoThey have Dart, which is alive and well after 9 years and Go, which is alive and well after 11 years. What point were you trying to make?
- user5994461 6y ago>>> Keep in mind, we run our production MySQL clusters with a pretty aggressive idle timeout (30s) Number one source of connectivity issue with MySQL. That setting alone is the source to every bug they encountered. It's marvelous the amount of applications that have a low timeout and wonder why the (idle) connections are dropped all the time. The timeout works fantastic when software prepare connection to the database, spend maybe 30 seconds initializing and doing other stuff, then send the query. The carefully prepared connection is guaranteed to be broken consistently. But the most impressive use case is with connection pools, whose purpose is to maintain idle connections, that gets butchered every 30 seconds. MySQL should remove this fatal setting and enable TCP keepalive by default. At least have mercy on the developers to set a sane default and cap the minimum value to 5 minutes.
- twhitmore 6y agoAgreed. This is fairly egregious misconfiguration. But it's also unsafe from the Go client/ connection pool to expect idle sockets to stay open indefinitely -- any firewall or network appliance traversal would be highly likely to invalidate that. Basic database connection pooling. Sigh.
- throwdbaaway 6y agoIt looks like they don't need the aggressive idle timeout anymore, now that they have ProxySQL for connection pooling: > ... ProxySQL, which is responsible for connection pooling ... https://github.blog/2020-03-26-february-service-disruptions-post-incident-analysis/ https://github.blog/2020-03-26-february-service-disruptions-... More worryingly, it looks like they just rewrote the authz functionality from ruby to golang, and have both the monolith and the microservice accessing the same database?
- EmielMols 6y agoThe work-around for the first issue might work well in practice, but due to asymmetry of tcp streams, is still open for a race where the server will idle-close a connection while a new (non-idempotent!) query is just in-flight. The correcter solution would let the client manage the idle-timeout and disconnect once reached. Depending a bit on how controlled the client is, this might be a good or bad idea. Note that this problem is very generalizable to http (1) upstream servers. If you need to support non-idempotent requests and want persistent connections to your upstream, it's not a good idea to have the upstream manage the idle timeout (and disconnect if reached). In practice, I would have the client manage an idle-timeout of 30s, and server of 40s as an extra protection against misbehaving clients.
- bgrainger 6y agoSome MySQL clients offer this; for example MySqlConnector has a `ConnectionIdleTimeout` setting that controls how long a connection can be alive in the pool: https://mysqlconnector.net/connection-options/ https://mysqlconnector.net/connection-options/ This is generally useful to "prune" the number of open connections in the connection pool after a burst of activity has happened, so I'm surprised more clients don't offer it; I couldn't find an equivalent setting on go-sql-driver/mysql or MySQL Connector/J.
- kevinherron 6y agoBug #1 doesn't seem solved. They just narrowed the window for the race condition. I don't see how you can fix this without the client first calling db.Ping() if within some proximity to its estimation of the connection's expiration.
- dancemethis 6y ago"We changed our proprietary code, thank you people working on Free Software btw" OK?
- sudhirj 6y agoIsn’t this clearly an explanation of a patch contributed upstream? Don’t think the connector is proprietary.
- markdog12 6y agoDumb question, but you can't listen for a close socket event in Go?
- tptacek 6y agoA TCP half-close, with a FIN? I think that's a limitation of the socket system call API itself.
- markdog12 6y agoYeah, I wasn't sure. I was looking at a Dart MySQL connection pool implementation, and that's what's done. socket.listen(onData, onDone: onSocketDone); onSocketDone() { _closed = true; } write(Buffer buffer) { if (_closed) throw StateError('Cannot write to socket, it is closed'); ...write } onData(RawSocketEvent event) { if (event == RawSocketEvent.READ_CLOSED) { _closed = true; } // Other events handled as well, READ, CLOSED, WRITE } But I don't know what's going on under the hood there.