Migration with lowercase 'current_timestamp' is not idempotent

[danbroooks]

On the project that I work on, we have a default field similar to this:

SomeEntity
  added    UTCTime    default=current_timestamp

When we run migrations on the project, we always get:

ALTER TABLE "some_entity" ALTER COLUMN "added" SET DEFAULT current_timestamp"

The column default gets set to CURRENT_TIMESTAMP, but every time we run migrations this ALTER TABLE appears.

I had a bit of a look at this, and I think this is happening because current_timestamp is in lowercase. We have other fields in our project like this, but the default is written like: CURRENT_TIMESTAMP.

I added some logging here:

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

Line 1568 in a4b4890

else let (acs, ats) = getAlters allDefs entity (newcols, udspair) old'

And looked in newcols and fst old' (the new columns vs existing columns), showing the field name and default value on the Column.

Here are the values that were logged out:

-- this is from `newcols`
(FieldNameDB {unFieldNameDB = "time_field"},Just "current_timestamp")
-- this is from `fst old`
(FieldNameDB {unFieldNameDB = "time_field"},Just "CURRENT_TIMESTAMP")

Ultimately these fields get compared here I think:

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

Line 1178 in a4b4890

if def == def'

So in this case, these are not matching.

I created a test to recreate this, there is an open PR with failing test here: #1533

I'm happy to work on resolving this, but the solution isn't super obvious to me. We could make this comparison case insensitive, and obviously that would need to be fixed across all backends (I've only looked at postgres, quite possibly other backends have a similar issue), but I'm not sure if that approach is the best, or if there would be possible issues introduced there.

Another approach I was thinking of would be to changing the cDefault field to parse certain fields, such as current_timestamp. Instead of:

cDefault :: Maybe Text

cDefault :: FieldDefault

-- where

data FieldDefault
  = FieldDefaultCurrentTime
  | FieldDefaultOther Text

And we parse both CURRENT_TIME / current_time (and any other similar) to a single value in this type.

Not sure about this approach either.

Happy to look to fix this with the right guidance, I don't think there is any actual issue here it's just an annoyance when running migrations, and I decided to actually look into this tonight 😄

[parsonsmatt]

This is an unfortunately common issue where a textual difference is seen as a mismatch, even though it's semantically equivalent.

It would be tricky to do the comparison in a case insensitive way: default="John" vs default="john" are obviously semantically different. But default=CURRENT_TIMESTAMP vs default=current_timestamp is not.

I'd suggest just changing the model definition to match what is returned by the database.

[jkeuhlen]

Just to add on here, this leads to some other oddities like field Int default=-1 working ~mostly correctly but you have to change it to be field Int default='-1'::integer to make the migration checking happy.