Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

database/sql: "Format3339Nano" not implemented; SQL TIMESTAMP -> Go Time doesn't work #24542

Closed
John-Nagle opened this issue Mar 26, 2018 · 12 comments

Comments

@John-Nagle
Copy link

@John-Nagle John-Nagle commented Mar 26, 2018

What version of Go are you using (go version)? go version go1.6.2 linux/amd64

Does this issue reproduce with the latest release? Yes

What operating system and processor architecture are you using (go env)? Ubuntu 16.04LTS

What did you do?

Go manual says at "https://golang.org/pkg/database/sql/#DB.Query":

Source values of type time.Time may be scanned into values of type *time.Time, *interface{}, *string, or *[]byte. When converting to the latter two, time.Format3339Nano is used. 

Format3339Nano doesn't seem to be documented anywhere, although it's mentioned in places that copy this SQL interface documentation. It's undefined in "time". Apparently that feature was never implemented.

Time.time should have
const Format3339Nano = "2006-01-02 15:04:05" // reference to RFC 3339
to match the documentation.

So, reading Time items doesn't work out of the box:

row := db.QueryRow("SELECT tripid, stamp FROM tripstodo WHERE TIMESTAMPDIFF(SECOND, stamp, NOW()) > ? ORDER BY stamp LIMIT 1", minSummarizeSecs)
var tripid string   // trip ID to be processed
var stamp time.Time // timestamp
err := row.Scan(&tripid, &stamp)

where "stamp" is of time MySQL TIMESTAMP, produces the error
*sql: Scan error on column index 1: unsupported Scan, storing driver.Value type []uint8 into type time.Time

The MySQL database module has an obscure variable to turn on a workaround: https://github.com/go-sql-driver/mysql#parsetime

but SQLite does not, according to this bug report from another package: https://github.com/jinzhu/gorm/issues/1361

This should Just Work.

@andybons andybons changed the title Documented "Format3339Nano" not implemented; SQL TIMESTAMP -> Go Time doesn't work database/sql: "Format3339Nano" not implemented; SQL TIMESTAMP -> Go Time doesn't work Mar 26, 2018
@andybons
Copy link
Member

@andybons andybons commented Mar 26, 2018

I believe it should be: time.RFC3339Nano, which is defined in the time package as "2006-01-02T15:04:05.999999999Z07:00", not "2006-01-02 15:04:05"

This is, in fact, what it uses when it encounters a *string, *[]byte or *RawBytes: https://golang.org/src/database/sql/convert.go#L260

But this seems separate from your issue of

var stamp time.Time // timestamp
err := row.Scan(&tripid, &stamp)

which shouldn’t hit the logic above.

Given this, what fix are you suggesting? (aside from correcting the documentation?)

@andybons andybons added this to the Go1.11 milestone Mar 26, 2018
@John-Nagle
Copy link
Author

@John-Nagle John-Nagle commented Mar 26, 2018

RFC3339Nano is different from the format used by MySQL. Error message from trying to convert a MySQL TIMESTAMP field to a Go time.Time using an explicit call to "time.Parse(time.RFC3339Nano, stamp)"

parsing time "2018-03-26 14:08:01" as "2006-01-02T15:04:05.999999999Z07:00": cannot parse " 14:08:01" as "T"

RFC3339Nano requires a "T" before the time. MySQL has a space. So that's a no-go workaround.

This is, in fact, what it uses when it encounters a *string, *[]byte or *RawBytes: https://golang.org/src/database/sql/convert.go#L260

The Format side (Go time.time -> SQL) is OK. It's the Parse side that's the problem.

Here's the real problem. Each SQL driver sends over the connection to the server some binary time format, which is supposed to be converted to the client platform (Go) time format down at that level. There's a little-known mode switch to enable this. See

https://github.com/go-sql-driver/mysql/blob/bc14601d1bd56421dd60f561e6052c9ed77f9daf/packets.go#L783

To make it work right, add "

?parseTime=true"

to the end of your DSN when opening the MySQL connection. Then, MySQL TIMESTAMP values convert to time.Time values properly. This is off by default. So, by default, the MySQL driver turns a TIMESTAMP into a string, The generic SQL driver has no parsing function to turn that into a time.Time.

Now, if you turn that switch on, TIMESTAMP to time.Time conversions work fine. But TIMESTAMP to Go "string" conversions then work differently. Now you get "2018-03-26T14:36:56Z" delivered into the Go string instead of "2018-03-26 14:08:01". So, if you turn that string on by default, it will probably break code that's expecting "2018-03-26 14:08:01" That's probably why somebody stuck in that mode switch.

So you're screwed. If you fix it right, you'll break code that's compatible with the legacy format.

There was discussion and a patch to fix all this back in 2016. See https://go-review.googlesource.com/c/go/+/18935 See that for background.

At least document this properly so it doesn't take two hours to figure it out. And tell the Gorm people, who've hit this problem.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2018

Change https://golang.org/cl/102607 mentions this issue: database/sql: fix docs to correctly refer to time.RFC3339Nano

@andybons
Copy link
Member

@andybons andybons commented Mar 26, 2018

Apologies as I’m still not clear on what documentation you’d like to see in Go’s sql package. It seems to me that this behavior is driver-specific and, as such, should be documented in the driver code.

perhaps @bradfitz can chime in.

gopherbot pushed a commit that referenced this issue Mar 26, 2018
It mentions time.Format3339Nano, which isn’t defined. The
underlying code uses time.RFC3339Nano.

Updates #24542

Change-Id: Ia34ae8b66427139d9005f902c2eb60aac4bfa8c6
Reviewed-on: https://go-review.googlesource.com/102607
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@John-Nagle
Copy link
Author

@John-Nagle John-Nagle commented Mar 26, 2018

TIMESTAMP and DATETIME should always convert to time.Time properly for all databases. No options should be needed to get that behavior. What happens on conversion to "string" is a separate issue.

@andybons
Copy link
Member

@andybons andybons commented Mar 26, 2018

I see. Thanks for being patient with me. So more concrete guidance for driver authors in these scenarios along the lines of what you said?

@John-Nagle
Copy link
Author

@John-Nagle John-Nagle commented Mar 27, 2018

The organizational dynamics of driver authors vs class-of-driver package authors I leave to you. From a user perspective, it should Just Work.

(I didn't classify this as a documentation bug. Gopherbot did. It's really a code bug. Classifier training problem?)

@andybons
Copy link
Member

@andybons andybons commented Mar 27, 2018

Well it was a documentation bug as you pointed out a mistake in our docs in addition to what we’re left with. Now it’s not one.

“Classifier” is far too generous a description for what GopherBot is doing ;)

@cznic
Copy link
Contributor

@cznic cznic commented Mar 27, 2018

From a user perspective, it should Just Work.

Let me please note that "Just Work" is a rather subjective term. I know of a company producing things said to "Just Work", but I am really not compatible with anything that company produces. So the quoted above has quite a different meaning to me.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 27, 2018

I'm going to throw this issue in the same bucket as #22544 and other issues.

One way to phrase this: some databases aren't picky about types (sqlite, mysql) and sometimes we return strings from a db that we want to interpret as a time. Doing this conversion generically (as we currently attempt to do) is fine for many cases, but it is more inefficient then optimal and misses edge cases.

In the next week or two I want to come up with a better Scan implementation. But it will probably be substantially different then the current implementation.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 19, 2018

Change https://golang.org/cl/107995 mentions this issue: database/sql: add the driver.ValueScanner interface

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 23, 2018
@kardianos
Copy link
Contributor

@kardianos kardianos commented Oct 27, 2018

I don't think this is issue is actionable. To resolve this, we would need to resolve #22544 .

@kardianos kardianos closed this Oct 27, 2018
@golang golang locked and limited conversation to collaborators Oct 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.