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

update to work with latest crystal #131

Closed
wants to merge 1 commit into from

Conversation

greenbigfrog
Copy link
Contributor

Do not merge. Wait for crystal release

@greenbigfrog greenbigfrog mentioned this pull request Jan 21, 2018
@will
Copy link
Owner

will commented Jan 23, 2018

Thank you, I'll keep an eye on this, but if you notice the new release first, please bump this.

@greenbigfrog greenbigfrog mentioned this pull request Feb 5, 2018
@bcardiff
Copy link
Collaborator

@greenbigfrog / @will if a non utc Time is sent as a param to be stored, pg will later return it with the original timezone or translated to UTC? In the specs I only see UTC times been used.

In the other drivers (at least for now) I am forcing that the time is saved in UTC since the DateTime in mysql has no timezone information.

Maybe in the future we could have which tz should be used by the db, but for now forcing UTC seems fine to me.

@will
Copy link
Owner

will commented May 29, 2018

I'm also pro-forcing-utc if that is at all reasonable. Right now looking a pq/param.cr, I think it assumes that all Times are utc, and just tacks on the iso 8601 z at the end. It probably needs to be changed to to add a call to #to_utc

@RX14
Copy link
Contributor

RX14 commented Jun 2, 2018

postgres has a timestamp without time zone and a timestamp with time zone datatype. For the ts with tz, we should store the correct timezone. For timestamp without time zone we should silently ignore the timezone and not convert to UTC.

@will
Copy link
Owner

will commented Jun 2, 2018

I'm not sure if doing that will help. As far as I can tell timestamp with timezone doesn't preserve the specific tz you give it, postgres instead converts it to UTC for you.

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.

When a timestamp with time zone value is output, it is always converted from UTC to the current timezone zone, and displayed as local time in that zone. To see the time in another time zone, either change timezone or use the AT TIME ZONE construct (see Section 9.9.3).

Inserting two different offsets into a tstz, they come back in my local laptop's tz:

will=# create table t(a timestamptz);
CREATE TABLE
will=# insert into t values ('2018-06-02 07:46:48.435705-07'), ('2018-06-02 07:46:48.435705-04');
INSERT 0 2
will=# select * from t
will-# ;
               a
-------------------------------
 2018-06-02 07:46:48.435705-07
 2018-06-02 04:46:48.435705-07
(2 rows)

whereas timestamp just ignores the extra info (which, as an aside, is behavior I don’t understand why anyone would want)

will=# create table t(a timestamp);
CREATE TABLE
will=# insert into t values ('2018-06-02 07:46:48.435705-07'), ('2018-06-02 07:46:48.435705-04');
INSERT 0 2
will=# select * from t
;
             a
----------------------------
 2018-06-02 07:46:48.435705
 2018-06-02 07:46:48.435705
(2 rows)

@RX14
Copy link
Contributor

RX14 commented Jun 2, 2018

Hmm, I thought tstz stored the explicit timezone the time was in. Thats not the case, so I was wrong.

@vladfaust
Copy link
Contributor

The time has come, my Travis builds fail with

require "./pg/*"
^
in lib/pg/src/pg/decoder.cr:4: undefined constant JSON::Type
  alias PGValue = String | Nil | Bool | Int32 | Float32 | Float64 | Time | JSON::Type | PG::Numeric
                                                                           ^~~~~~~~~~

@will
Copy link
Owner

will commented Jun 15, 2018

I cherry-picked this into master and released v0.15.0. Thanks!

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.

None yet

5 participants