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

Datetime should be mapped to timesamp "with time zone" instead of "without time zone" #12

Open
fleebzz opened this issue Oct 13, 2016 · 6 comments

Comments

@fleebzz
Copy link

fleebzz commented Oct 13, 2016

Hi, there is currently no way to create a column with the type timestamp with time zone.

When I look into the code I found that datetime is mapped to timestamp. So no matter if I define a timestamp or a datetime I will have a timezone.

In the MySQL documentation for DATETIME you can see that this type of column takes care of local time zone.

Is there something wrong to map datetime to timestamp with time zone ?

I tried to update the mapping locally and all work fine for me. Do you see any issue doing this ?

If not I can submit a PR with these changes.

Thanks.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@wzrdtales
Copy link
Member

Did you missed the repo? You're talking about MySQL but this is the pg repo and not MySQL. But generally we're always open for PRs though.

Is there something wrong to map datetime to timestamp with time zone ?

Yes, timestampt is actually a type, with timezone is just an option and thus not the mapping should resolve to this, but it would make sense to have the option to enable this.

And I guess you mean:
https://github.com/db-migrate/pg/blob/master/index.js#L57

Then again, this is the postgres driver
https://www.postgresql.org/docs/9.1/static/datatype-datetime.html
And for postgres there is only timestamp though.

If you want to open a PR adding the option to add with time zone to a timestamp column, this would be welcome. If you have any other question, don't hesitate to ask :)

@fleebzz
Copy link
Author

fleebzz commented Oct 13, 2016

No I'm on the right repo. I just took MySQL as an example for this issue but I use PG. And I know that timestamp is a type.

In MySQL there is a difference between timestamp (w/o time zone) and datetime (with time zone).

But, in this repo, there is no difference between timestamp and datetime because its mapped to timestamp.

So I suggest to map datetime to timestamp with time zone instead of timestamp for PG.

I'm not sure it's a good idea to add a new option. If it's the only solution, it should be done in db-migrate repo and mapped in every db-migrate-driver repo. If the option is only available here (for pg driver) my migrations won't be db-agnostic anymore...

@wzrdtales
Copy link
Member

wzrdtales commented Oct 13, 2016

Sry, I hope this answer is not too long...

If the option is only available here (for pg driver) my migrations won't be db-agnostic anymore...

That is not true entirely. Options that do not exist on other dbs, just get dropped, see for example the constraints, there are already some, that do only apply to some dbs. And on the other side: There is no way to be completely db-agnostic and this is also something you probably never want. If you choose a database, you do this normaly for a reason and don't just randomly pick one. And this also means, that you want to use the database you choosed and all its capabilities, which automatically puts you in a state of incompatibility to other dbs. If you want everything completely generic, then there can't be any background behind your choice of the database you used, as there are always some trade offs and you probably did not choose a specific database b/c you don't want to actually use it.

But to come back to the main topic:

I'm not sure it's a good idea to add a new option. If it's the only solution, it should be done in db-migrate repo and mapped in every db-migrate-driver repo.

Yes it is a good idea as this is driver specific and does not apply globally, but it is not a good idea to map the datatype to something not matching the expectations anymore. And that is actually something you currently ask for:

Changing a datatype to something that is not anymore just a datatype. And this can't and wont be done for multiple reasons.To just call out the most significant one:
This would be a breaking change. And therefore a no go and on the other hand, no one is ever going to expect, that a datatype would behave differently from just being a datatype. This is one of the cases, where there shouldn*t be to much opinions in the implementation, but just the raw unmodified datatype. Everything else are additions though.

It is not that I do not understand what you're thinking of or that I think that genericity is bad, but it is very limited to a certain point. As soon as you actually want to use something, that is specific, you can't really bind yourself to those limits anymore. I personally always see genericity as a side goal of migrations. Genericity does on the one hand allow to switch over to another database way easier and on the other hand it can allow to quite easily write migrations for multiple target dbs, but just with limitations. As soon as you pass those limitations, you need to put some extra efforts, to make those usable on other databases, as said earlier: Everything comes with a trade off. And in this case you made the comparison with MySQL, but this is not a good candidate to compare against. There are so many different DBs out there, therefore it does not make sense to compare them in that way. But what we actually can do is to take the actual ANSI SQL Standard, also to mention that this doesn't really apply to migrations as we also handle NoSQL DBs, and the Standard don't make a difference actually:

The data types DATE, TIME, and TIMESTAMP are collectively referred to as datetime types. Values of datetime types are referred to as datetimes.

And:

Datetimes only have absolute meaning in the context of additional
information. Time zones are political divisions of the earth's
surface that allow the convention that time is measured the same
at all locations within the time zone, regardless of the precise
value of "sun time" at specific locations. Political entities often
change the "local time" within a time zone for certain periods of
the year, e.g., in the summer. However, different political enti-
ties within the same time zone are not necessarily synchronized in
their local time changes. When a datetime is specified (in SQL-data
or elsewhere) it has an implied or explicit time zone specifier as-
sociated with it. Unless that time zone specifier, and its meaning,
is known, the meaning of the datetime value is ambiguous.

Therefore, datetime data types that contain time fields (TIME and
TIMESTAMP) are maintained in Universal Coordinated Time (UTC), with
an explicit or implied time zone part.

Source:
http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt

So in a short version:

We can't do this as this would:

  • be a breaking change (therefore a no go)
  • mismatch expectations
  • not be a datatype anymore if options already apply
  • be to opinionated (a standard is actually opinionated too, but it is a result of collective effort and accepted by many people)

@fleebzz
Copy link
Author

fleebzz commented Oct 14, 2016

Ok I think I understand your point of view. Thanks for the (long) explanation 😉

So I tried to use type : 'timestamp with time zone' in my migration and it worked but got a warning message saying [WARN] Using unknown data type TIMESTAMP WITH TIME ZONE.

Do you think it's good idea to stay like this ? Or should I add an option with another method ? I don't see any way to do something like this in the doc of column specs.

@wzrdtales
Copy link
Member

[WARN] Using unknown data type TIMESTAMP WITH TIME ZONE

This just tells you: Hey, you're using something custom, be aware of that fact. That is nothing negative though.

Do you think it's good idea to stay like this ? Or should I add an option with another method ? I don't see any way to do something like this in the doc of column specs.

I think, it would be better to use the normal datatype and add the possibility to specify options for that datatype. This is something that is planned anyway though. Especially for datatypes like enum and many more, if you want to take a stab at this feel free to do so. I will first finish my work on the docs and tests I am currently investing, before taking on on new tasks though :)

@yangg
Copy link
Contributor

yangg commented Sep 25, 2017

#17 add support for column specs { timezone: true }
This issue can be close now @wzrdtales

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

3 participants