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

Timestamps round trip incorrectly on non UTC systems #1195

Closed
wbl opened this issue Apr 26, 2022 · 22 comments
Closed

Timestamps round trip incorrectly on non UTC systems #1195

wbl opened this issue Apr 26, 2022 · 22 comments

Comments

@wbl
Copy link

wbl commented Apr 26, 2022

When I am on a system not set to UTC, and store a timestamp to a database via the database/sql compatible interface, the value in the database is stored with the same numeric value but the zone set to UTC. When reading back the timestamp stored in the DB is correctly reconstituted, but alas this is not the same as the value that went in.

@wbl
Copy link
Author

wbl commented Apr 26, 2022

Setting the location to UTC on the timestamp does work, so this is not really a function of the system timezone but the way timestamps are serialized.

@jackc
Copy link
Owner

jackc commented Apr 27, 2022

Is the database column a timestamp or timestamptz? Can you make a small reproduction case for this?

@wbl
Copy link
Author

wbl commented May 5, 2022

It's timestamp. I can make a small reproduction sometime: been a bit busy lately.

@frederikhors
Copy link

The same problem here (from ent/ent#2692):

The column created in Postgres is:

used_at timestamp with time zone NOT NULL,

The value in Postgres is saved without timezone:

2022-06-30 22:49:03.970913+00

I'm using PG in Docker and using the PG query:

show timezone

I get:

Etc/UTC

I get from Ent (using pgx stdlib) the value:

2022-07-01T00:49:03.970913+02:00

This is a mess on the clients (where an UTC value is expected).

Using pgdriver/pq I get the UTC value from DB.

Am I wrong somewhere?

@frederikhors
Copy link

frederikhors commented Jun 27, 2022

I tried using this connection with this code too:

postgres://postgres:postgres@localhost/project?sslmode=disable&timezone=UTC"
import (
	"database/sql"
	_ "github.com/jackc/pgx/v4/stdlib"
)

conn, err := sql.Open("pgx", "postgres://postgres:postgres@localhost/project?sslmode=disable&timezone=UTC")
//handle err

The problem is here.

I need a way to get back from DB the UTC values (that are laready stored in the DB).

@frederikhors
Copy link

@jackc can you please help us here? 🙏

@frederikhors
Copy link

I asked on SO too: https://stackoverflow.com/questions/72771272/how-to-setup-pgx-to-get-utc-values-from-db.

They suggest a custom type. More code, more issues!

@wbl did you find a way?

@jackc
Copy link
Owner

jackc commented Jul 1, 2022

Okay, I see what's going on here. The SO thread basically has the answer, but here's a little more of the explanation of why.

timestamptz doesn't store time zones. PostgreSQL always stores UTC and automatically converts on the way in and out. Except when using the binary format. Then it always sends an int64 of the number of microseconds since Y2K (similar to Unix time). There is no way to use the PostgreSQL session time zone (the text format is better, but it isn't perfect either).

That leaves us with the choice of using UTC or using the time.Local / ENV["TZ"]. It's an arbitrary choice, but its been that way for at least 5 years. I can't see breaking compatibility now.

@wbl
Copy link
Author

wbl commented Jul 1, 2022

So Go time representation stores the timezone explicitly. Code that is working is setting the location to UTC on retrieval and adjusting the offsets on insertion. If the returned times are set to the UTC location, it won't break code that is working.

If we don't want to fix it via what would technically be a breaking change, adding documentation could help.

@frederikhors
Copy link

@jackc I'm asking you an OPTIONAL config to return UTC as stored in DB. Please. No breaking.

@jackc
Copy link
Owner

jackc commented Jul 1, 2022

@jackc I'm asking you an OPTIONAL config to return UTC as stored in DB. Please. No breaking.

It already is optional. Just not with a config change. Create a new type that has pgtype.Timestamptz as its underlying type. Delegate all calls to pgtype.Timestamptz. For DecodeText and DecodeBinary run your time zone conversion code after the delegated call. See pgtype.JSONB for an example of pgx doing that style itself. pgtype.JSONB just adds a tiny bit of code to the original pgtype.JSON. Then use RegisterDataType to replace the default timestamptz data type.

@univerio
Copy link

AFAICT a custom type works in all situations except when using the database/sql interface to scan into a time.Time, because stdlib does not respect ConnInfo when returning values from the driver.Rows interface and there's no easy way to change this behavior short of reimplementing the whole driver.Connector interface:

pgx/stdlib/sql.go

Lines 717 to 726 in d42b399

case pgtype.TimestamptzOID:
var d pgtype.Timestamptz
scanPlan := ci.PlanScan(dataTypeOID, format, &d)
r.valueFuncs[i] = func(src []byte) (driver.Value, error) {
err := scanPlan.Scan(ci, dataTypeOID, format, src, &d)
if err != nil {
return nil, err
}
return d.Value()
}

@dedalusj
Copy link
Contributor

It already is optional. Just not with a config change. Create a new type that has pgtype.Timestamptz as its underlying type. Delegate all calls to pgtype.Timestamptz. For DecodeText and DecodeBinary run your time zone conversion code after the delegated call.

@jackc what is the minimal approach to implement the solution above in pgx v5?

I would like to wrap existing types as much as possible and just take the scan result time and convert it to UTC.

I thought the following could be a path

type timestamptzCodec struct {
	pgtype.TimestamptzCodec
}

func (t *timestamptzCodec) PlanScan(m *pgtype.Map, oid uint32, format int16, target any) pgtype.ScanPlan {
	plan := t.TimestamptzCodec.PlanScan(m, oid, format, target)
	if plan == nil {
		return nil
	}
	return &timestamptzPlan{plan}
}

type timestamptzPlan struct {
	pgtype.ScanPlan
}

func (t *timestamptzPlan) Scan(src []byte, target any) error {
	err := t.ScanPlan.Scan(src, target)
	fmt.Printf("Target info %v %T\n", target, target)
	return err
}

However target in the Scan method is of type *pgtype.timeWrapper so it cannot be cast to *time.Time and it can't be manipulated directly.

Is there a simpler solution?

@jackc
Copy link
Owner

jackc commented Oct 1, 2022

@dedalusj

I think what you would want to do is to create your own wrapper type that implements pgtype.TimestamptzScanner with your preferred time zone handling.

Then your choice is whether to have a custom Codec that handles *time.Time before delegating to the builtin Codec or you could insert logic in pgtype.Map.TryWrapScanPlanFuncs. For that approach you would write a function like pgtype.TryWrapBuiltinTypeScanPlan that detects time.Time and wraps it with your type. Prepend that function to the pgtype.Map.TryWrapScanPlanFuncs to give it priority over over the automatic wrapping of a *time.Time with *pgtype.timeWrapper.

@i0n
Copy link

i0n commented Oct 23, 2022

Just ran into this issue.

Currently if you define a timestampz field then marshal and unmarshal as normal using pgx you will get incorrect data in memory (but correct in db) unless your local timezone happens to match the timezone of the entry.

Setting the TZ environment variable does not seem to change this.

@broady
Copy link

broady commented Dec 5, 2022

Ran into this issue too. I would have expected the driver to convert time.Time values to UTC. I don't care about storing time zone, just want the same timestamp returned (e.g., Unix() is equivalent for what I stored and retrieved).

@Emixam23
Copy link

Emixam23 commented Feb 27, 2023

Hey there :)

I am having the same issue..

Whats weird is that, when I use sql.Open or sqlx.Connect, everything works fine.
However, when using OpenDB (passing it a driver), it always converts the time to Local on scan instead of UTC.. I am working with timestampz

Any idea?

Thanks

@louisrli
Copy link

I seem to be running into this issue with a query that looks like this, where created_at is a timestamp column

SELECT id, created_at, title FROM sheet WHERE created_at <= $1

When I pass in time.Now() or time.Now().UTC() to the query, it's not working as expected. I'll post a workaround here if I figure it out

@fantapop
Copy link

@dedalusj Did you end up getting this working? I'm sure a working example could help out a lot of folks.

@astockwell
Copy link

astockwell commented Oct 25, 2023

Ran into this as well with created_at / updated_at columns with GORM and beyond. Interesting because, even if you aggressively set the timezone in the pg database, set it in the connection string, and override your system global time zone, and set them all to UTC... you get back timestamps that appear identical for all intents and purposes (time values are identical down to nanoseconds, tz are identical), but are not quite equal in the eyes of the compiler (although they are time.Time.Equal()). Those of us who are lazy, terrible, no-good devs who use reflect.DeepEquals() seem to be out of luck in the current situation, barring writing our own scan implementations.

jackc added a commit that referenced this issue Mar 16, 2024
If ScanLocation is set, it will be used to convert the time to the given
location when scanning from the database.

The Codec interface is now implemented by *pgtype.TimestamptzCodec
instead of pgtype.TimestamptzCodec. This is technically a breaking
change, but it is extremely unlikely that anyone is depending on this,
and if there is downstream breakage it is trivial to fix.

#1195
#1945
jackc added a commit that referenced this issue Mar 16, 2024
If ScanLocation is set, the timestamps will be assumed to be in the
given location when scanning from the database.

The Codec interface is now implemented by *pgtype.TimestampCodec instead
of pgtype.TimestampCodec. This is technically a breaking change, but it
is extremely unlikely that anyone is depending on this, and if there is
downstream breakage it is trivial to fix.

#1195
#1945
@jackc
Copy link
Owner

jackc commented Mar 16, 2024

This issue was originally filed with pgx v4. I don't see making any changes there.

But think that PR #1948 would solve all the issues raised here for pgx v5.

It allows specifying the desired time.Location that should be used for scanned values on the Codec level. It also should solve the issue of the time location being nil vs. time.UTC.

		conn.TypeMap().RegisterType(&pgtype.Type{
			Name:  "timestamptz",
			OID:   pgtype.TimestamptzOID,
			Codec: &pgtype.TimestamptzCodec{ScanLocation: time.UTC},
		})

Please review and give feedback.

jackc added a commit that referenced this issue May 8, 2024
If ScanLocation is set, it will be used to convert the time to the given
location when scanning from the database.

The Codec interface is now implemented by *pgtype.TimestamptzCodec
instead of pgtype.TimestamptzCodec. This is technically a breaking
change, but it is extremely unlikely that anyone is depending on this,
and if there is downstream breakage it is trivial to fix.

#1195
#1945
jackc added a commit that referenced this issue May 8, 2024
If ScanLocation is set, the timestamps will be assumed to be in the
given location when scanning from the database.

The Codec interface is now implemented by *pgtype.TimestampCodec instead
of pgtype.TimestampCodec. This is technically a breaking change, but it
is extremely unlikely that anyone is depending on this, and if there is
downstream breakage it is trivial to fix.

#1195
#1945
@jackc
Copy link
Owner

jackc commented May 8, 2024

#1948 has been merged. This should resolve this issue.

@jackc jackc closed this as completed May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests