Improve handling of PostgreSQL intervals #1604

tfausak · 2025-07-10T15:39:13Z

Persistent uses the PgInterval type to handle PostgreSQL intervals. Unfortunately it's not a good fit for that. On the Haskell side, PgInterval is just a wrapper around NominalDiffTime, which represents some amount of picoseconds. On the PostgreSQL side, an interval has 32 bits of precision for months and days together with 64 bits of precision for microseconds. So converting between PgInterval and interval is not total in either direction.

I created a new Interval type in my postgresql-simple-interval library. It accurately models PostgreSQL's interval type and provides most of the instances and functions that you'd want. https://hackage-content.haskell.org/package/postgresql-simple-interval-0.2025.8.27/docs/Database-PostgreSQL-Simple-Interval.html#t:Interval

This pull request updates persistent-postgresql to use the Interval type from postgresql-simple-interval rather than PgInterval. It leaves PgInterval for backwards compatibility, but I would recommend deprecating and eventually removing it.

These issues and PRs are related:

tfausak · 2025-07-10T18:39:02Z

persistent-postgresql/Database/Persist/Postgresql/Internal.hs

-
 instance PGTF.ToField PgInterval where
-    toField (PgInterval t) = PGTF.toField t
+    toField = PGTF.toField . fromMaybe (error "PgInterval.toField") . pgIntervalToInterval


This (and toPersistValue) should perhaps saturate/clamp the interval rather than throwing an impure exception. For example if you try to insert PgInterval 9223372036855, the existing toPersistValue method will generate "9223372036855s", which Postgres will complain about:

SqlError { sqlState = "22015" , sqlExecStatus = FatalError , sqlErrorMsg = "interval field value out of range: \"9223372036855s\"" , sqlErrorDetail = "" , sqlErrorHint = "" }

To clarify, that example is what happens with the current version of Persistent if you try to insert a PgInterval that's too big for PostgreSQL to handle. With these changes, an impure exception would be thrown from Haskell instead.

I understand that the current behaviour is not ideal, but is it necessary to change the To/FromField instances for PgInterval here? I'd prefer that we leave it as-is if we can (especially if the aim is for downstream users to stop using PgInterval), since it's possible that there are downstream users who are relying on the error being thrown at the database level in this case.

Unfortunately I think it is necessary to change the instance. The builtinGetters are type directed, so I'm forced to decode a PostgreSQL interval into a Haskell Interval. I could introduce a type that's essentially Either PgInterval Interval, but that feels like it's not worth the effort. I can't imagine that someone is relying on the behavior of PgInterval throwing a SqlError when it's too big.

cabal.project

tfausak · 2025-07-10T20:29:10Z

persistent-postgresql/Database/Persist/Postgresql/Internal.hs

 -- | Represent Postgres interval using NominalDiffTime
 --
 -- @since 2.11.0.0
 newtype PgInterval = PgInterval {getPgInterval :: NominalDiffTime}


I would suggest that longer term the PgInterval type should be deprecated and ultimately removed.

I agree - can we add a comment here explaining why it's best avoided and that it might be deprecated in the future?

Actually, never mind, I changed my mind - I think it's a valid use case to say that you want to eg store intervals in the database that should always be convertable to NominalDiffTime so that you can interact with them using that type on the Haskell side. I think we should call out the fact that the conversion isn't always possible in the docs for this type, but I also think there are probably lots of use cases where this tradeoff is acceptable and the risk of accidentally creating intervals which can't be converted to NominalDiffTime is low - the fact that this is the type that actually exists in this library right now and has done for years is evidence of this, I think.

cabal.project

tfausak · 2025-07-14T14:52:25Z

.github/workflows/haskell.yml

          restore-keys: |
            ${{ runner.os }}-${{ matrix.ghc }}-${{ hashFiles('cabal.project.freeze') }}
            ${{ runner.os }}-${{ matrix.ghc }}-
-      - run: cabal v2-build all --disable-optimization --only-dependencies $CONFIG


I removed this because I was seeing an error:

Cannot select only the dependencies (as requested by the '--only-dependencies' flag), the package persistent-2.17.1.0 is required by a dependency of one of the other targets.

I think that happened because postgresql-simple-interval depends on persistent, which is obviously part of this project. So Cabal can't build only the dependencies, since that would require also building persistent.

tfausak · 2025-09-04T19:04:00Z

Here are some examples of behavior before and after these changes:

Click to reveal extensions and imports.

:set -XOverloadedStrings
:set -XTypeApplications

import Control.Monad.Logger as L
import Database.Persist.Sql as S
import Database.Persist.Postgresql as P
import Database.PostgreSQL.Simple.Interval as I

L.runStderrLoggingT (P.withPostgresqlConn "user=postgres" (\ c -> S.runSqlConn (S.rawSql @(S.Single P.PgInterval) "..." [...]) c))

-- PgInterval can't represent days.
rawSql "select '1 day'::interval" []

-- before:
-- *** Exception: ConversionFailed {errSQLType = "interval", errSQLTableOid = column number 0 is out of range 0..-1
-- Nothing, errSQLField = "column number 0 is out of range 0..-1
-- ", errHaskellType = "PgInterval", errMessage = ":: Failed reading: satisfyWith"}

-- after:
-- *** Exception: user error (invalid interval)

-- PgInterval can't represent months.
rawSql "select '1 month'::interval'" []

-- before:
-- *** Exception: ConversionFailed {errSQLType = "interval", errSQLTableOid = column number 0 is out of range 0..-1
-- Nothing, errSQLField = "column number 0 is out of range 0..-1
-- ", errHaskellType = "PgInterval", errMessage = ":: Failed reading: satisfyWith"}

-- after:
-- *** Exception: user error (invalid interval)

-- PgInterval loses precision.
rawSql "select ?::interval" [toPersistValue $ PgInterval 0.0000009]

-- before:
-- [Single {unSingle = PgInterval {getPgInterval = 0.000001s}}]

-- after:
-- [Single {unSingle = PgInterval {getPgInterval = 0s}}]

-- PgInterval overflows.
rawSql "select ?::interval" [toPersistValue $ PgInterval 9223372036854.775808]

-- before:
-- *** Exception: SqlError {sqlState = "22015", sqlExecStatus = FatalError, sqlErrorMsg = "interval field value out of range: \"9223372036854.775808s\"", sqlErrorDetail = "", sqlErrorHint = ""}

-- after:
-- *** Exception: PgInterval.toPersistValue

And for completeness, here's how the new Interval type handles those scenarios:

-- Interval can represent days.
rawSql "select '1 day'::interval" []
-- [Single {unSingle = MkInterval {months = 0, days = 1, microseconds = 0}}]

-- Interval can represent months.
rawSql "select '1 month'::interval" []
-- [Single {unSingle = MkInterval {months = 1, days = 0, microseconds = 0}}]

-- Interval cannot lose precision (correct by construction).
rawSql "select ?::interval" [toPersistValue $ MkInterval 0 0 1]
-- [Single {unSingle = MkInterval {months = 0, days = 0, microseconds = 1}}]

-- Interval cannot overflow (correct by construction).
rawSql "select ?::interval" [toPersistValue $ MkInterval 0 0 9223372036854775807]
-- [Single {unSingle = MkInterval {months = 0, days = 0, microseconds = 9223372036854775807}}]

hdgarrood · 2025-09-04T16:31:45Z

persistent-postgresql/Database/Persist/Postgresql/Internal.hs

 instance PersistFieldSql PgInterval where
    sqlType _ = SqlOther "interval"

+pgIntervalToInterval :: PgInterval -> Maybe Interval.Interval


Can you reproduce the relevant part of your PR comment here, and also for the function in the other direction?

Also, rather than having these here, I think I would prefer that postgres-simple-interval provide conversions to and from NominalDiffTime - in the case where you need to deal with an interval in both the database and in Haskell code, it's going to be a lot more comfortable to work with NominalDiffTime on the Haskell side.

I just opened a PR for adding these conversions to postgresql-simple-interval: MercuryTechnologies/postgresql-simple-interval#24

I think it may not be great for an interval to be backed by a NominalDiffTime - the syntax allows, for example, INTERVAL '1 month' where the exact NominalDiffTime this corresponds to would differ based on both operation and starting month. Same for INTERVAL '1 year' though that is down to leap seconds and leap days.

Indeed, that's the whole point of this PR! Although for posterity it's interval '1 day' and interval '1 month' that the existing PgInterval can't handle. interval '1 year' is the same as interval '12 months'.

Is a one-day interval on the postgres side ever different from nominalDay?

oh lol you answered on the other thread

hdgarrood · 2025-09-05T13:51:19Z

persistent-postgresql/Database/Persist/Postgresql/Internal.hs

+intervalToPgInterval :: Interval.Interval -> Maybe PgInterval
+intervalToPgInterval interval
+    | Interval.months interval /= 0 = Nothing
+    | Interval.days interval /= 0 = Nothing


It feels to me like it should be possible to handle nonzero days values here - can we essentially do days * 86400 + seconds?

We could, but that has the potential to be incorrect in the presence of leap seconds. Since the current PgInterval doesn't handle days at all, I figured there's no reason to do a best effort conversion here.

Makes sense, ty

hdgarrood · 2025-09-05T13:54:46Z

persistent-postgresql/Database/Persist/Postgresql/Internal.hs

-
 instance PGTF.ToField PgInterval where
-    toField (PgInterval t) = PGTF.toField t
+    toField = PGTF.toField . fromMaybe (error "PgInterval.toField") . pgIntervalToInterval


I understand that the current behaviour is not ideal, but is it necessary to change the To/FromField instances for PgInterval here? I'd prefer that we leave it as-is if we can (especially if the aim is for downstream users to stop using PgInterval), since it's possible that there are downstream users who are relying on the error being thrown at the database level in this case.

hdgarrood · 2025-09-05T13:55:21Z

persistent-postgresql/Database/Persist/Postgresql/Internal.hs

 -- | Represent Postgres interval using NominalDiffTime
 --
 -- @since 2.11.0.0
 newtype PgInterval = PgInterval {getPgInterval :: NominalDiffTime}


I agree - can we add a comment here explaining why it's best avoided and that it might be deprecated in the future?

hdgarrood

I don't think I have the commit bit on this repo but the code looks good to me now. Thanks! I think all that's left is a changelog update?

persistent-postgresql/Database/Persist/Postgresql/Internal.hs

parsonsmatt · 2025-09-08T16:36:11Z

persistent-postgresql/test/PgIntervalTest.hs

+        prop "interval round trips" $ \(m, d, u) -> runConnAssert $ do
+            let
+                expected =
+                    IntervalDb . Interval.MkInterval m d $
+                        clamp (-microsecondLimit) microsecondLimit u
+            key <- insert expected
+            actual <- getJust key
+            liftIO $ actual `shouldBe` expected


See also the round trip tests in postgresql-simple-interval's suite: https://github.com/MercuryTechnologies/postgresql-simple-interval/blob/7d58fbe45b6a2306d4ae025e71bfa196b1d8254f/source/test-suite/Main.hs#L428-L429

parsonsmatt · 2025-09-08T16:39:33Z

persistent-postgresql/ChangeLog.md

+    * Changed the representation of intervals to use the `Interval` type from [the `postgresql-simple-interval` package](https://hackage.haskell.org/package/postgresql-simple-interval).
+      This changes the behavior of `PgInterval` for very small and very large values.
+    * Previously `PgInterval 0.000_000_9` would be rounded to `0.000_001` seconds, but now it is truncated to 0 seconds.
+    * Previously `PgInterval 9_223_372_036_854.775_808` would overflow and throw a SQL error, but now it saturates to `9_223_372_036_854.775_807` seconds.


Is this true for all values greater than 9_223_372_036_854.775_807? ie "this is the maximum number of seconds that PgInterval can now represent?"

Hm, no, that's me being confused. Previously, you could construct an interval that was huge in Haskell, and you would error in SQL when you try to write it. Now, instead of a thrown exception, you get a clamped value, which is a problem if you were dealing with (consults calculator) >=292,271 years and relying on an exception to prevent you from doing this.

Correct. Previously if you tried to insert PgInterval 9223372036854.775808, you'd get a SQL error.

postgres=# select interval '9223372036854.775808 seconds'; ERROR: interval field value out of range: "9223372036854.775808 seconds" LINE 1: select interval '9223372036854.775808 seconds'; ^

parsonsmatt · 2025-09-08T16:43:33Z

persistent-postgresql/ChangeLog.md

+* [#1604](https://github.com/yesodweb/persistent/pull/1604)
+    * Changed the representation of intervals to use the `Interval` type from [the `postgresql-simple-interval` package](https://hackage.haskell.org/package/postgresql-simple-interval).
+      This changes the behavior of `PgInterval` for very small and very large values.
+    * Previously `PgInterval 0.000_000_9` would be rounded to `0.000_001` seconds, but now it is truncated to 0 seconds.


Is this Haskell behavior or database behavior? And is this something that would be observable on write or read?

From reading the interval docs, I'm guessing that you mean that we'd round PgInterval up when writing to the database, and that the database cannot represent a value like 0.000_000_9 in the first place. So values of 500-999 picoseconds are going to behave differently.

The answer is a little complicated. postgresql-simple-interval uses realToFrac to convert from Pico to Micro (source). That truncates extra precision:

ghci> realToFrac (0.0000009 :: Pico) :: Micro 0.000000

I'm ... not totally sure what PostgreSQL does, actually. I thought it parsed the input as a double and then rounded, but I'm not seeing that behavior experimentally:

-- 2025-09-09 10:43:48.239 CDT [50979] LOG: starting PostgreSQL 17.6 on aarch64-apple-darwin24.6.0, compiled by clang version 19.1.7, 64-bit -- psql (17.6) -- good, rounds down select interval '0.0000004 seconds'; -- 00:00:00 -- good, rounds up select interval '0.0000006 seconds'; -- 00:00:00.000001 -- good, rounds half to even select interval '0.0000005 seconds'; -- 00:00:00 -- bad, rounds half to odd select interval '0.0000015 seconds'; -- 00:00:00.000001 -- bad and weird, inconsistent with previous select interval '0.00000151 seconds'; -- 00:00:00.000002 select interval '1 second' * 0.0000015; --00:00:00.000002

Co-authored-by: Matt Parsons <[email protected]>

parsonsmatt · 2025-09-09T16:05:59Z

tagged and released, thank you!

tfausak and others added 4 commits July 10, 2025 10:36

Use Interval type from postgresql-simple-interval

794a268

Merge branch 'master' into tfausak/2025-07-10-interval

7b275fa

Allow building with older versions of base

208c58c

Allow older bytestring

f8a9aa6

tfausak mentioned this pull request Jul 10, 2025

Support older versions of GHC MercuryTechnologies/postgresql-simple-interval#8

Merged

Fix imports

9594877

tfausak commented Jul 10, 2025

View reviewed changes

cabal.project Outdated Show resolved Hide resolved

tfausak added 3 commits July 10, 2025 15:24

Avoid generating invalid intervals

080ef87

Merge branch 'master' into tfausak/2025-07-10-interval

e313366

Fix formatting

3ea86b0

tfausak commented Jul 10, 2025

View reviewed changes

Use upstream Persistent instances

87387b0

tfausak commented Jul 11, 2025

View reviewed changes

cabal.project Outdated Show resolved Hide resolved

tfausak and others added 2 commits July 11, 2025 10:21

Upgrade to postgresql-simple-interval 0.2025.7.11

2e6a88a

Avoid building dependencies separately

15ac84b

This comment was marked as outdated.

Sign in to view

tfausak mentioned this pull request Jul 11, 2025

Change render format MercuryTechnologies/postgresql-simple-interval#11

Merged

Upgrade to postgresql-simple-interval 0.2025.7.12

c2821cc

tfausak marked this pull request as draft July 12, 2025 15:10

tfausak and others added 4 commits July 12, 2025 10:40

Limit microsecond range

a93db23

Fix clamp

aec020a

Fix tests

517e0c7

Remove unnecessary dependency

cb00db5

tfausak commented Jul 14, 2025

View reviewed changes

tfausak marked this pull request as ready for review July 14, 2025 14:53

Upgrade to postgresql-simple-interval 0.2025.8.27

027f902

tfausak changed the title ~~Use Interval type from postgresql-simple-interval~~ Improve handling of PostgreSQL intervals Sep 4, 2025

hdgarrood reviewed Sep 5, 2025

View reviewed changes

tfausak added 2 commits September 8, 2025 09:03

Upgrade to postgresql-simple-interval 0.2025.9.5

913f4ad

Add more documentation to PgInterval

0d27391

hdgarrood reviewed Sep 8, 2025

View reviewed changes

tfausak and others added 4 commits September 8, 2025 10:54

Update change log

bcc8404

Update change log

4f5f5a7

Update postgresql-simple-interval version constraint

398e084

Rerun CI

d235d4d

tfausak requested a review from parsonsmatt September 9, 2025 14:48

parsonsmatt approved these changes Sep 9, 2025

View reviewed changes

Move @since annotation to the end

285647a

Co-authored-by: Matt Parsons <[email protected]>

parsonsmatt merged commit 1d109e6 into yesodweb:master Sep 9, 2025
10 checks passed

Improve handling of PostgreSQL intervals #1604

Improve handling of PostgreSQL intervals #1604

Uh oh!

Conversation

tfausak commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

tfausak commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

parsonsmatt commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tfausak commented Jul 10, 2025 •

edited

Loading