lib / pq

Pure Go Postgres driver for database/sql
https://pkg.go.dev/github.com/lib/pq
MIT License
9.01k stars 910 forks source link

Scan naive dates as UTC, matching stdlib #956

Closed akshayjshah closed 2 years ago

akshayjshah commented 4 years ago

When scanned into a time.Time, we currently assign temporal types without timezone information a time.Location of time.FixedLocation("", 0) (i.e., an unnamed location with zero offset from UTC). This PR changes behavior to use time.UTC instead.

This change is desirable because it makes our behavior match the standard library's time package, which typically uses time.FixedLocation only when there is an explicit offset provided. When there's no timezone data, the standard library assumes UTC. (See time.Parse for an example.)

This change shouldn't be too disruptive to well-behaved client code. The returned time.Time structs still represent the same instant in time, and they're equivalent when compared using time.Equal. The discussion on #329 includes many people using reflect.DeepEqual to compare time.Times, but that should have become much less common after the introduction of monotonic time in 2017, the addition of time.Equal, and the slow spread of github.com/google/go-cmp/cmp in tests.

This PR fixes #329 and supersedes the now-ancient #328. I pulled in @e-dard's commit, so he should still get credit for filing the initial issue and adding a failing test case.

cc @stgarrity, who's also interested

akshayjshah commented 4 years ago

The failing test looks like it may be related to the race described in #563. Perhaps @mjibson or another maintainer can re-run the failing portion of the build?

amenzhinsky commented 4 years ago

I'm sorry but this doesn't solve the problem, the default timezone should fallback to the local timezone not UTC, plus it can be easily controlled with TZ env var.

For example querying a database with a default timezone, say +10 with SELECT NOW()::timestamp results in the same timestamp but with UTC or +0 tz now, local time in go app would be +20 already.

amenzhinsky commented 4 years ago
# select now();
              now
-------------------------------
 2020-06-16 17:36:56.045896+03

Ends up 2020-06-16 17:36:56.045896 +0000 +0000 in go when it should be +3.

johto commented 4 years ago

I'm sorry but this doesn't solve the problem, the default timezone should fallback to the local timezone not UTC, plus it can be easily controlled with TZ env var.

That's just one opinion. If we don't know what the time zone is, any guess we do is going to be incorrect for some people. Go chooses to guess UTC, postgres uses the TimeZone setting.

For example querying a database with a default timezone, say +10 with SELECT NOW()::timestamp results in the same timestamp but with UTC or +0 tz now, local time in go app would be +20 already.

So don't do that.

amenzhinsky commented 4 years ago

That's just one opinion. If we don't know what the time zone is, any guess we do is going to be incorrect for some people. Go chooses to guess UTC, postgres uses the TimeZone setting.

What I'm saying is that all apps should default to the system timezone, that's what initdb does, TimeZone is an optional setting

johto commented 4 years ago

What I'm saying is that all apps should default to the system timezone, that's what initdb does, TimeZone is an optional setting

You should tell that to Go then as well:

In the absence of a time zone indicator, Parse returns a time in UTC.

akshayjshah commented 4 years ago

@amenzhinsky, there's already a long-ish discussion of this behavior on #329. Let's keep discussion of the desired behavior on that thread, so it benefits from context. Discussion here would be more helpful if it focused on this PR's implementation of the desired behavior.

If you don't have a chance to read all of #328, the most relevant portion is @johto's comment from 2015:

I think we should use the session time zone.

No, that would be a huge backwards compatibility breakage.

akshayjshah commented 2 years ago

Seems like this isn't going anywhere; I'm closing this to get it off my list of open PRs.