-
Notifications
You must be signed in to change notification settings - Fork 7
Commit
- Loading branch information
There are no files selected for viewing
4 comments
on commit 138423f
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.
Hmmm. I'm not 100% sure that the timezone handling is correct here. Are you sure that you should convert the datetime to UTC? (See format_value.dart:84)
Another option would be if the DateTime object is a localtime, to pass the localtime including the timezone name. The database will then convert the timestamp to UTC when storing it, and display it in the localtime of the database server. I think this is the idea behind the timestamptz type, but I'm not very familiar with it.
Here are some snippets from the Dart and Postgresql docs.
https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart-core.DateTime
A DateTime object is in the local time zone unless explicitly created in the UTC time zone. Use isUtc to determine whether a DateTime object is based in UTC. Use the methods toLocal and toUtc to get the equivalent date/time value specified in the other time zone. Use timeZoneName to get an abbreviated name of the time zone for the DateTime object.
http://www.postgresql.org/docs/9.3/static/datatype-datetime.html#AEN5808
For timestamp with time zone, the internally stored value is always in UTC (Universal Coordinated Time, traditionally known as Greenwich Mean Time, GMT). An input value that has an explicit time zone specified is converted to UTC using the appropriate offset for that time zone. If no time zone is stated in the input string, then it is assumed to be in the time zone indicated by the system's TimeZone parameter, and is converted to UTC using the offset for the timezone zone.
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.
Converting to UTC is a trick to simplify the formatting. Alternatively, we can format it under the local time and with a proper offset. However, they shall be the same, since Postgresql will convert it to UTC anyway and store it in UTC (for timestamptz).
Please be noticed that I just tested it on my applications. It could be just a quick fix for my environment. Please consider the pull request just a suggestion. It is more than welcome if you reject it and come out with a more complete solution.
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.
Note - if you're looking for the quickest solution, you can pass the timestamp as a string instead of a DateTime object.
Regarding the UTC conversion, I think there is a problem if the application server and database server are running in different timezones. However I'm not sure if that is a valid use case or not. I might check what other database drivers do, and match their behaviour.
Another problem I noticed, is if the timestamptz value is null, then connection.dart:535 will throw.
I can come up with a fix for these issues if you like, but it will require waiting for a day or two. Let me know what you'd rather do. I'm happy either way. Thanks for your help. ;)
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.
Passing a string is something I'd like to avoid:)
if the application server and database server are running in different timezones.
Good question. In my implementation, it shall work if timestamptz is specified (such as @time:timestampz
), since it forced to convert to UTC (with a proper time zone name, Z). However, if timestamptz is not specified, formatValue has no information but either assume it is timestamp or timestamptz. If assuming timestamp (which is the current implementation), it won't work since it is formatted in local time. If assuming timestamptz, I'm not sure if it broke the code using timestamp (I didn't use timestamp at all).
I can come up with a fix for these issues...
Yes, please. I can count on my aggressive and less stable version before the official one comes out.
I just noticed that this doesn't actually specify the millisecond field:
new DateTime(1979, 12, 20, 9, 0, 12);
You may have meant to write:
new DateTime(1979, 12, 20, 9, 0, 0, 12);
This is also buggy in the timestamp test, I didn't notice it in the other commit. I even copy and pasted it myself. Doh!