-
Notifications
You must be signed in to change notification settings - Fork 116
Remove conversion from Int to PGInt4. Add from Int32 to PGInt4.
#110
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
base: master
Are you sure you want to change the base?
Conversation
Whereas the size of `PGInt4` is fixed to 4 bytes, the size of `Int` is machine dependent (likely to be 8 bytes in today's 64-bit operating systems). This difference will lead to silently losing information when inserting an `Int` greater than 2147483647 to the database. By using conditional compilation (CPP) we could decide where to map `Int` to either `PGInt4` or `PGInt8`. However, that is problematic, because we could still silently lose information if we write to the database from a computer where `Int` maps to `PGInt8`, and then try to recover that information on another computer where `Int` maps to `PGInt4`. Not to mention that the computer where `Int` maps to `PGInt8` will be expecting an `bigint` column in the database, where other computers will be expecting a `integer` column. Maintainability and compatibility wise, the sane solution is to avoid providing support for converting the machine dependent `Int` into either `PGInt4` or `PGInt8`, and instead focus on covering the explicit `Int64`, `Int32`, and `Int16` conversions. This is the same approach used by PostgreSQL, which does not offer primitive integers of machine dependent sizes: http://www.postgresql.org/docs/9.4/static/datatype-numeric.html This deliberately backwards incompatible change abandons support for converting the `Int` data type to `PGInt4`, and instead makes explicit the mapping from `Int32` to `PGInt4`, following the already existing mapping between PGInt8` and `Int64`. The conversion on the other direction is preserved and improved. That is, one is able to convert from `PGInt2` or `PGInt4` to `Int`, and on platforms where `Int` is 8 bytes wide, converting from `PGInt8` to `Int` is also supported.
|
Actually, I think this can be made somewhat more backwards compatible. My previous comment was wrong:
As long as we keep the conversions exclusive (that is, |
|
In 9223756 I made the change I mentioned in my last comment. |
|
I should note that we could have an even more backwards compatible solution, albeit a bit more sophisticated: class ToPGInt8 a where pgInt8 :: a -> PGInt8
instance ToPGInt8 Int32 where pgInt8 = ...
instance ToPGInt8 Int64 where pgInt8 = ...
instance ToPGInt8 Int where pgInt8 = ...
class ToPGInt4 a where pgInt4 :: a -> PGInt4
instance ToPGInt4 Int32 where pgInt4 = ...
#if WORD_SIZE_IN_BITS == 32
instance ToPGInt4 Int where pgInt4 = ...
#endifIs this preferred? Notice that since |
|
I don't want to change the behaviour of existing conversions but I am happy to document any strange behaviour and add safer versions. |
|
@tomjaguarpaw well, the current situation is beyond just “strange”: it will lead to loss on information on 64-bit systems. I for one have already added a wrapper around all this (i.e., around Can't we just remove that and keep the sane versions of these functions in the next major version change? Keep in mind that a backwards incompatible change like this one will not alter previous behaviors silently, instead it will lead to failures at compile time that will be easy to fix, so it should not be a big deal from that point of view. |
|
I see your point, but I'm still going to take some convincing. In any case, there's nothing wrong with adding the correct conversions and remove the wrong ones later. If you submit two separate PRs then I'll merge the first and we can discuss the second. |
(This is first part of tomjaguarpaw#110.)
Whereas the size of
PGInt4is fixed to 4 bytes, the size ofIntismachine dependent (likely to be 8 bytes in today's 64-bit operating
systems). This difference will lead to silently losing information when
inserting an
Intgreater than 2147483647 to the database.By using conditional compilation (CPP) we could decide where to map
Intto eitherPGInt4orPGInt8. However, that is problematic,because we could still silently lose information if we write to the
database from a computer where
Intmaps toPGInt8, and then try torecover that information on another computer where
Intmaps toPGInt4. Not to mention that the computer whereIntmaps toPGInt8will be expecting an
bigintcolumn in the database, where othercomputers will be expecting a
integercolumn.Maintainability and compatibility wise, the sane solution is to avoid
providing support for converting the machine dependent
Intinto eitherPGInt4orPGInt8, and instead focus on covering the explicitInt64,Int32, andInt16conversions. This is the same approachused by PostgreSQL, which does not offer primitive integers of machine
dependent sizes:
http://www.postgresql.org/docs/9.4/static/datatype-numeric.html
This deliberately backwards incompatible change abandons support for
converting the
Intdata type toPGInt4, and instead makes explicitthe mapping from
Int32toPGInt4, following the already existingmapping between PGInt8
andInt64`.The conversion on the other direction is preserved and improved. That
is, one is able to convert from
PGInt2orPGInt4toInt, and onplatforms where
Intis 8 bytes wide, converting fromPGInt8toIntis also supported.