-
-
Notifications
You must be signed in to change notification settings - Fork 140
feat: support dynamic timestamp ScanLocation via DSN timezone #311
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
Conversation
0c899b2
to
c901b0e
Compare
go.mod
Outdated
@@ -1,16 +1,16 @@ | |||
module gorm.io/driver/postgres | |||
|
|||
go 1.19 | |||
go 1.21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, please avoid making this change to ensure compatibility with projects that are still running on older versions of Go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a custom TimestampCodec with ScanLocation set to the parsed time zone is only supported starting from pgx version 5.6.0, which requires Go 1.20.0. Since the go.mod file in this PR currently specifies Go 1.19, the Go version must be upgraded to at least 1.20.0. Without this change, this PR will fail to compile after merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jinzhu If there’s no plan to upgrade the minimum Go version requirement in the near future, I’m thinking it might be better to close this PR for now. Instead, I can submit a new PR that adds support for passing stdlib.OptionOpenDB through configuration. That way, existing projects using older versions of Go will continue to work as expected, while users who need the newer pgx features can opt in by passing stdlib.OptionOpenDB themselves after upgrading their dependencies. Let me know if that sounds good to you! |
In that case, let’s go ahead and upgrade the minimum supported Go version. Users who rely on older versions can continue using the previous release |
In that case, should we upgrade the minimum Go version to 1.20, or would it make more sense to go with a higher version? Just for reference:
Happy to align with whichever version you’d prefer to support going forward. |
The minimum version can support this feature should be ok. |
Hi @jinzhu Thank you again for reviewing and merging my previous pull request! I’m reaching out to ask if there’s any plan to release a new version soon. The Goravel project depends on this package, and we’re hoping to include some recent bug fixes in our upcoming release. Really appreciate your work on this project! |
Just released version v1.6.0 |
Hello @almas-x, @jinzhu, I think this MR broke something, I bumped to v1.6.0 and I can't start the app anymore: 2025/05/28 20:17:02 donetick.com/core/internal/database/database.go:25
[error] failed to initialize database, got error unknown time zone Asia/Shanghai
2025-05-28T20:17:02.242Z warn database/database.go:31 failed to open database: unknown time zone Asia/Shanghai
donetick.com/core/internal/database.NewDatabase
donetick.com/core/internal/database/database.go:31
reflect.Value.call
reflect/value.go:584
reflect.Value.Call
reflect/value.go:368
go.uber.org/dig.defaultInvoker
go.uber.org/dig@v1.19.0/container.go:257
go.uber.org/dig.(*constructorNode).Call
go.uber.org/dig@v1.19.0/constructor.go:198
go.uber.org/dig.paramSingle.Build
go.uber.org/dig@v1.19.0/param.go:288
go.uber.org/dig.paramList.BuildList
go.uber.org/dig@v1.19.0/param.go:151
go.uber.org/dig.(*constructorNode).Call
go.uber.org/dig@v1.19.0/constructor.go:160
go.uber.org/dig.paramSingle.Build
go.uber.org/dig@v1.19.0/param.go:288
go.uber.org/dig.paramList.BuildList
go.uber.org/dig@v1.19.0/param.go:151
go.uber.org/dig.(*Scope).Invoke
go.uber.org/dig@v1.19.0/invoke.go:123
go.uber.org/dig.(*Container).Invoke
go.uber.org/dig@v1.19.0/invoke.go:83
go.uber.org/fx.runInvoke
go.uber.org/fx@v1.24.0/invoke.go:109
go.uber.org/fx.(*module).invoke
go.uber.org/fx@v1.24.0/module.go:335
go.uber.org/fx.(*module).invokeAll
go.uber.org/fx@v1.24.0/module.go:321
go.uber.org/fx.New
go.uber.org/fx@v1.24.0/app.go:507
main.main
donetick.com/core/main.go:55
runtime.main
runtime/proc.go:283 Here's the related code: https://github.com/donetick/donetick/blob/b9a6e177eefdc605dedbc5320f0d93d6573d1db6/internal/database/database.go#L23-L27 I can open an issue if you prefer. |
Same problem as above, it looks like new changes bugged
Downgraded back to |
I just tried something and I now have a it working on 1.6.0, I'm running the program in a docker container which seems to miss timezone information (alpine based), I added the following import ( ![]() |
Background
By default, pgx parses PostgreSQL timestamp (without time zone) fields as UTC.
However, in many real-world scenarios, timestamps are stored in local time (e.g., Asia/Shanghai).
To address this, the timezone parameter is now extracted from the DSN and used to set
the ScanLocation of the pgtype.TimestampCodec, ensuring correct interpretation of timestamp values.
What did this pull request do?
timezone
value from the DSN, if provided*time.Location
TimestampCodec
withScanLocation
set to the parsed locationThis ensures that timestamp parsing behavior is aligned with the configured session time zone.
User Case Description
In many PostgreSQL schemas,
timestamp
fields (without time zone) are used to store datetime values in local time, such asAsia/Shanghai
.However, by default, the pgx driver parses these values as UTC, which can lead to incorrect time interpretation in applications — for example, an event saved as
2024-01-01 08:00:00
may be misinterpreted as2024-01-01 08:00:00 UTC
instead ofAsia/Shanghai
.This becomes problematic when:
To solve this, the DSN now accepts a
timezone
parameter (e.g.,timezone=Asia/Shanghai
), which is used to configure pgx’sTimestampCodec.ScanLocation
.This ensures that timestamp values are interpreted in the correct local time zone, aligning with how the data was originally stored.
This change improves consistency, prevents subtle bugs, and reduces the need for manual
.In(time.Local)
conversions in application code.Related issue #199