-
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 = ...
#endif Is 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
PGInt4
is fixed to 4 bytes, the size ofInt
ismachine 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 eitherPGInt4
orPGInt8
. However, that is problematic,because we could still silently lose information if we write to the
database from a computer where
Int
maps toPGInt8
, and then try torecover that information on another computer where
Int
maps toPGInt4
. Not to mention that the computer whereInt
maps toPGInt8
will be expecting an
bigint
column in the database, where othercomputers 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 eitherPGInt4
orPGInt8
, and instead focus on covering the explicitInt64
,Int32
, andInt16
conversions. 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
Int
data type toPGInt4
, and instead makes explicitthe mapping from
Int32
toPGInt4
, following the already existingmapping between PGInt8
and
Int64`.The conversion on the other direction is preserved and improved. That
is, one is able to convert from
PGInt2
orPGInt4
toInt
, and onplatforms where
Int
is 8 bytes wide, converting fromPGInt8
toInt
is also supported.