Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

k0001
Copy link
Contributor

@k0001 k0001 commented Sep 15, 2015

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 PGInt8andInt64`.

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.

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.
@k0001
Copy link
Contributor Author

k0001 commented Sep 15, 2015

Actually, I think this can be made somewhat more backwards compatible. My previous comment was wrong:

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.

As long as we keep the conversions exclusive (that is, Int maps either to PGInt4 or to PGInt8, but not to both), and combine this with the already proposed conditional compilation from PGInt4 or PGInt8 to Int, then we should be fine. I don't know if that's worth it. What do you think?

@k0001
Copy link
Contributor Author

k0001 commented Sep 15, 2015

In 9223756 I made the change I mentioned in my last comment.

@k0001
Copy link
Contributor Author

k0001 commented Sep 15, 2015

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 WORD_SIZE_IN_BITS is usually 64 nowadays, I suspect most previous uses of pgInt4 :: Int -> PGInt4 would fail to compile anyway, so I'm not sure if this is worth it.

@k0001 k0001 mentioned this pull request Sep 20, 2015
@tomjaguarpaw
Copy link
Owner

I don't want to change the behaviour of existing conversions but I am happy to document any strange behaviour and add safer versions.

@k0001
Copy link
Contributor Author

k0001 commented Sep 30, 2015

@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 Constant), so that I can avoid accidentally falling into this trap.

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.

@tomjaguarpaw
Copy link
Owner

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.

duairc added a commit to duairc/opaleye that referenced this pull request Apr 1, 2016
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.

2 participants