Store timezone-less dates in local time instead of UTC #238

Merged
merged 4 commits into from Apr 22, 2013

Projects

None yet

6 participants

@cdauth
Contributor
cdauth commented Jan 14, 2013

Supersedes #237

@cdauth
Contributor
cdauth commented Jan 14, 2013

Somehow the existing tests fail for me because of timezone issues. Right now I don’t have the time to dig into that in order to write a test for this issue.

@brianc
Owner
brianc commented Jan 14, 2013

yeah once we get the test this can be merged no problemo

@brianc
Owner
brianc commented Jan 24, 2013

just an update: awaiting tests

@strk
Contributor
strk commented Feb 18, 2013

another update: I still don't see tests...

@strk
Contributor
strk commented Apr 8, 2013

Any luck with the test ?

@cdauth
Contributor
cdauth commented Apr 10, 2013

I added a test for the issue. It passes in JavaScript and native mode, but it fails in binary JavaScript mode. Any ideas what might be different in binary mode? In type-coercion-tests.js, the date test is only run in non-binary mode, why is that?

@cdauth
Contributor
cdauth commented Apr 10, 2013

Fixed, the parsing of timezone-less dates was different in binary mode than in text mode.

@brianc
Owner
brianc commented Apr 11, 2013

@cdauth awesome thanks I'll try to get to this one today.

@Tol1
Tol1 commented Apr 22, 2013

What's status with this? This issue kinda prevents me upgrading...

@brianc
Owner
brianc commented Apr 22, 2013

Whoops - sorry I didn't get to this one. Got distracted by something, and then my mind went dark. Merging now.

@brianc brianc merged commit 8a2e864 into brianc:master Apr 22, 2013

1 check passed

default The Travis build passed
Details
@brianc
Owner
brianc commented Apr 22, 2013

Pushed out to NPM. Thanks for the passing tests! 👍

@thomseddon

@brianc @cdauth Sorry if I have misunderstood something, but is there a reason why this diff only covers the binary parser as I seem to be having the same issue when the text parser is being used?

@cdauth
Contributor
cdauth commented Jun 18, 2013

Hm, there are several commits in this pull request, one of them fixes the issue with the binary parser, another fixes the one with the text parser.

@thomseddon

Ah, quite right, assume you're referring to the changes to what's returned by prepareValue(). Issue with this is that it's only called for prepared statements, so unprepared text queries will not be transformed.

I was about to build a diff similar to the one you wrote for the binary parsers, for the text parser. This would solve the issue at source?

@cdauth
Contributor
cdauth commented Jun 18, 2013

Hm, do you have some example code how you are experiencing this problem? I don’t see how unprepared text queries should be transformed in any way. The problem occurred when a JavaScript date object was converted into a text query, so I don’t see how the issue can affect unprepared text queries at all.

@thomseddon
var pg = require('pg');

var client = new pg.Client('pg://postgres@127.0.0.1:5432/node_postgres');

var build = function (done) {
    client.connect(function (err) {
        if (err) {
            console.log(err);
            return client.end();
        }

        client.query('CREATE TABLE IF NOT EXISTS times(manual TIMESTAMP WITHOUT TIME ZONE, ' +
                'auto TIMESTAMP WITHOUT TIME ZONE DEFAULT CURRENT_TIMESTAMP); TRUNCATE times;',
                function (err) {
            if (err) {
                console.log(err);
                return client.end();
            }
            done();
        });
    });
};

var date = new Date();
build(function () {
    client.query('INSERT INTO times(manual) VALUES($1)', [date], function (err, res) {
        client.query('SELECT manual, auto, CURRENT_TIMESTAMP as now from times',
                function (err, res) {
            client.end();
            if (err) return console.log(err);

            res.rows[0].input = date;
            console.log(res.rows[0]);
        });
    });
});

For me (currently in BST (GMT+1)) this outputs:

{ manual: Tue Jun 18 2013 14:47:05 GMT+0100 (BST),
  auto: Tue Jun 18 2013 13:47:05 GMT+0100 (BST),
  now: Tue Jun 18 2013 14:47:05 GMT+0100 (BST),
  input: Tue Jun 18 2013 14:47:05 GMT+0100 (BST) }

Clearly the auto column is wrong, what it's actually returning is UTC/GMT, it should be returning the same as the other three values, right?

@strk
Contributor
strk commented Jun 18, 2013

I confirm postgresql drops the timezone info when casting time to a timestamp without timezone.
Easy test:

=# SELECT current_timestamp, current_timestamp::timestamp;
-[ RECORD 1 ]----------------------
2013-06-18 16:36:35.48274+02
2013-06-18 16:36:35.48274
@spollack
Contributor

The same issue happens when postgres casts to date -- timezone is lost, which can cause it to land on the wrong date (off by one).

@strk
Contributor
strk commented Jun 18, 2013

In any case, node-postgres interprets both dates as being in the correct timezone, which I think is correct.
In the first case it'll use the timezone reported by postgresql, in the second case it'll use the server timezone.
I think it's the correct behavior.

@spollack
Contributor

The problem i've seen here is that if your client is running in some arbitrary time zone (say UTC - 7:00), and your server/database are running in UTC, and then the client passes in a javascript Date object via pg that gets cast to a postgres date type, then if you are in the last 7 hours of the local day (i.e. in the next day UTC), you get the wrong date.

@strk
Contributor
strk commented Jun 18, 2013

@spollack, isn't the problem fixed by using "timestamp with timezone", aka "timestamptz" ?

@spollack
Contributor

@strk for timestamps, yes, changing from "timestamp" to "timestamp with timezone" fixes this. However, for postgres type "date", which we also use in our schema, we run into this problem.

@thomseddon

@strk Quite right, changing to "with timezone" fixes it as postgres respects the input timezone

I think the behaviour of node-postgres, with respect to timezone handling, should be explicitly clarified/documented...This seems to have been a recurring issue so I think having a definitive spec would help further discussion.

@brianc
Owner
brianc commented Jun 18, 2013

AFAIK the date handling is in line with PostgreSQL's requirements in that dates coming out of postgres are converted into the local time if there is no timezone specified.

If you're having problems with date converting I recommend specifying timezone with your dates. That way there's no ambiguity or data loss. If you're okay not knowing to which timezone the date belongs you're gonna have to write app level code at some level to ensure they're stored & retrieved correctly and in accord with your app's view of dates. e.g. "always convert dates to UTC before storing them. Always re-convert dates to UTC after storing them."

@spollack
Contributor

@brianc i agree, this has to be managed by app code for type "date" (since unfortunately postgres has no "date with timezone" type).

@brianc
Owner
brianc commented Jun 18, 2013

Yeah, that's a bummer. Since it comes out as a javascript date either way why not store is timestamptz and then use something like http://momentjs.com/ to help ease the conversion in app code? Dealing with timezones is always a monster & definitely not easy with JavaScript's pretty limited date object.

@spollack
Contributor

@brianc yes i may switch the datatype.

we started with "date" because we are modeling something that is truly a value per day (and where time of day is meaningless), but i could just ignore the timestamptz time component or zero it out.

i agree that timezones are always a monster!! and definitely momentjs is the way to go with date handling.

@thomseddon

@brianc Thanks, suppose the issue still with "convert dates to UTC for storage" + timestamp without time zone is, as you said, that when they come out the timezone is wrong as node-postgres assumes they are in local time.

It would be difficult to get around this as you would then have to know what was timestamp without time zone and what was timestamp with timezone (e.g. my "CURRENT_TIMESTAMP AS now") to know what to correct on the way out.

Is the solution just to use timestamp with timezone?

@cdauth
Contributor
cdauth commented Jun 18, 2013

I think the problem is that JavaScript itself does not have a type that matches the postgres timestamp without timezone type, as Date objects behave exactly like timestamp with timezone fields. I don’t know if it actually makes sense to represent timestamp without timezone fields as Date objects in node-postgres.

But I also can’t think of any cases where it would make sense to use timestamp without timezone fields at all, I think most of the times they are used because of a misunderstanding that makes people think that timestamp with timezone fields actually store the entered timezone, even though they just convert the entered dates to UTC on writing and to the local timezone on reading.

@thomseddon

I can vouch for that misunderstanding, that is the behaviour of the
similarly named type in Oracle however as it turns out Postgres always
stores as UTC the end result of this episode has just been to switch
everything to WITH TIME ZONE.
On 18 Jun 2013 18:01, "Candid Dauth" notifications@github.com wrote:

I think the problem is that JavaScript itself does not have a type that
matches the postgres timestamp without timezone type, as Date objects
behave exactly like timestamp with timezone fields. I don’t know if it
actually makes sense to represent timestamp without timezone fields as Date
objects in node-postgres.

But I also can’t think of any cases where it would make sense to use
timestamp without timezone fields at all, I think most of the times they
are used because of a misunderstanding that makes people think that
timestamp with timezone fields actually store the entered timezone,
even though they just convert the entered dates to UTC.


Reply to this email directly or view it on GitHubhttps://github.com/brianc/node-postgres/pull/238#issuecomment-19625152
.

@spollack spollack referenced this pull request Sep 19, 2013
Closed

Wrong timestamp! #429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment