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

Fix handling of timestamp fields without timezone #237

Closed
wants to merge 1 commit into from
Closed

Fix handling of timestamp fields without timezone #237

wants to merge 1 commit into from

Conversation

cdauth
Copy link
Contributor

@cdauth cdauth commented Jan 13, 2013

Commit b7fd9a5 fixes issue #225 but breaks the handling of timestamp fields that do not contain a timezone. The values returned by Postgres are always UTC, but the text parser assumes them to be in the timezone of the Node VM.

@freewil
Copy link

freewil commented Jan 14, 2013

Can you make a test for this issue?

@brianc
Copy link
Owner

brianc commented Jan 14, 2013

I think we need to have a discussion with @strk - especially with regards to #226

I initially had timezones always converted to UTC on fetch if they had no timezone specified, but the manual specifies they're assumed to be in the timezone on the reader (citation needed).

@strk
Copy link
Contributor

strk commented Jan 14, 2013

http://www.postgresql.org/docs/8.0/static/datatype-datetime.html

This says they are assumed to be in the timezone of the writer:
""If a literal is not explicitly indicated as being of timestamp with time zone, PostgreSQL will silently ignore any time zone indication in the literal. That is, the resulting date/time value is derived from the date/time fields in the input value, and is not adjusted for time zone.""

This says they are assumed to be in the timezone of the reader:
""Conversions between timestamp without time zone and timestamp with time zone normally assume that the timestamp without time zone value should be taken or given as timezone local time. A different zone reference can be specified for the conversion using AT TIME ZONE.""

I think it makes sense and the rationale is: in absence of a specified timezone we'll always assume times are local.
The bad consequence is that the same stored time may identify a different point in time depending on reader's timezone, but that's why "timestamp with timezone" exist...

@cdauth
Copy link
Contributor Author

cdauth commented Jan 14, 2013

The problem with the current implementation is that the way dates are stored in the database differs from the way they are read from there. When you create a new Date(0) object, write it to a TIMESTAMP WITHOUT TIMEZONE field and then read it again, you will receive a new Date(-3600000) object if your computer is set to timezone +01:00.

If I understand you correctly, you propose to change prepareValue() in utils.js instead of the text parser?

@strk
Copy link
Contributor

strk commented Jan 14, 2013

Why do you receive a different date than you insert ?
Is it changed before getting in the db ?
I don't see anything done to date objects in the current prepareValue:
https://github.com/brianc/node-postgres/blob/v0.11.0/lib/utils.js#L98

Can you verify why where that value changes ?

@cdauth
Copy link
Contributor Author

cdauth commented Jan 14, 2013

As far as I know, JSON.stringify() returns the date as UTC in ISO format.

@strk
Copy link
Contributor

strk commented Jan 14, 2013

On Mon, Jan 14, 2013 at 07:29:46AM -0800, cdauth wrote:

As far as I know, JSON.stringify() returns the date as UTC in ISO format.

Oh, I got it. Ok.

Then I guess it should be some subsequent code changing it
back to local time IFF the target field type is timestamp without
timezone, because prepareValue doesn't seem to know about target
field name, right ?

@cdauth
Copy link
Contributor Author

cdauth commented Jan 14, 2013

Hm, I guess if prepareValue() returned the date in the local timezone instead of UTC, things would be fine. If I understand the docu correctly, on a field without timezone, postgres would just ignore the timezone.

@strk
Copy link
Contributor

strk commented Jan 14, 2013

On Mon, Jan 14, 2013 at 07:35:19AM -0800, cdauth wrote:

Hm, I guess if prepareValue() returned the date in the local timezone instead of UTC, things would be fine. If I understand the docu correctly, on a field without timezone, postgres would just ignore the timezone.

Uhm, you're right:

strk=# select '2013-01-14 16:41:58.623017+01'::timestamp;
         timestamp
----------------------------
 2013-01-14 16:41:58.623017
(1 row)

strk=# select '2013-01-14 16:41:58.623017+01'::timestamptz;
          timestamptz
-------------------------------
 2013-01-14 16:41:58.623017+01
(1 row)

As long as we can get that kind of representation..

Issue #225 caused such dates to be read, but not stored in local time.
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

Successfully merging this pull request may close these issues.

4 participants