Postgrex expected %NaiveDateTime{}

[benwilson512]

Hey folks. Under Postgrex 0.13 and Ecto 2.2.10 the following works:

iex(3)> now = DateTime.utc_now                                                               
#DateTime<2018-10-15 21:28:04.061218Z>
iex(4)> Repo.all from u in Sensetra.Model.User, where: fragment("? >= ?", u.updated_at, ^now)
[]

Now under 0.14 and Ecto 3.0.0-rc.0, it works when using the column:

iex(3)> now = DateTime.utc_now                                             
#DateTime<2018-10-15 21:24:24.886315Z>
iex(4)> Repo.all from u in Sensetra.Model.User, where: u.updated_at >= ^now
[]

But no longer when using a fragment:

iex(5)> Repo.all from u in Sensetra.Model.User, where: fragment("? >= ?", u.updated_at, ^now)
** (DBConnection.EncodeError) Postgrex expected %NaiveDateTime{}, got #DateTime<2018-10-15 21:24:24.886315Z>. Please make sure the value you are passing matches the definition in your table or in your query or convert the value accordingly.
    (sensetra) /Users/ben/src/sensetra/deps/postgrex/lib/postgrex/type_module.ex:713: Sensetra.PostgresTypes.encode_params/3
    (postgrex) lib/postgrex/query.ex:62: DBConnection.Query.Postgrex.Query.encode/3
    (db_connection) lib/db_connection.ex:1074: DBConnection.encode/5
    (db_connection) lib/db_connection.ex:1172: DBConnection.run_prepare_execute/5
    (db_connection) lib/db_connection.ex:1268: DBConnection.run/6
    (db_connection) lib/db_connection.ex:480: DBConnection.parsed_prepare_execute/5
    (db_connection) lib/db_connection.ex:473: DBConnection.prepare_execute/4
    (ecto_sql) lib/ecto/adapters/sql.ex:531: Ecto.Adapters.SQL.execute!/4

The column type in the database is timestamp without time zone and in the ecto schema it's :utc_datetime_usec.

Looking at the postgrex code it seems like it's expecting the database types timestamp vs timestampz to match up with NaiveDateTime vs DateTime but that would be a mistake. Neither column type stores timezone information, it's entirely about whether the values are returned in the client local timezone or not: https://stackoverflow.com/questions/5876218/difference-between-timestamps-with-without-time-zone-in-postgresql

By specifying the :utc_datetime_usec schema type I'm stipulating that the column stores UTC datetimes, so it ought to accept Elixir DateTime.

[wojtekmach]

The underlining reason this crashes is we always send timestamps as NaiveDateTime which maps to timestamp [without time zone] field [1]. With fragments we can't always know the type so to fix the issue you'd have to cast explicitly:

where: fragment("? >= ?", u.updated_at, type(^now, :utc_datetime_usec))

I actually run into a similar issue recently, but I don't yet know if we can improve it.

[1] https://github.com/elixir-ecto/ecto_sql/blob/v3.0.0-rc.0/lib/ecto/adapters/postgres/connection.ex#L1085:L1088

[benwilson512]

Hey @wojtekmach. I suppose what I'm unclear on is how it figures out that it's a timestamp column but not that it's a :utc_datetime_usec column as well. The only way it would know we're using a timestamp column is from u.updated_at, but if we know u.updated_at presumably it would also know that the schema already specifies the type as :utc_datetime_usec which would let us avoid casting.

What approach was used in Postgrex 0.13?

[wojtekmach]

The error comes not from the u.updated_at but from the ^now part of the query. I believe one of the design goals was that when we interpolate values inside fragments we send them straight to the adapter, and because Postgrex has strict mapping NaiveDateTime <> timestamp without timezone and DateTime <> timestamp with time zone we do need to send it as NaiveDateTime.

What approach was used in Postgrex 0.13?

when I was testing this I think I saw it sent as {{2018, 1, 1}, {0, 0, 0}} to the adapter regardless if it was a NaiveDateTime or DateTime, although I'm not sure if Ecto or Postgrex or both changed in that regard.

[benwilson512]

because Postgrex has strict mapping NaiveDateTime <> timestamp without timezone and DateTime <> timestamp with time zone

If it has that mapping then that seems like an error. timestamp with time zone does NOT include timezone information. It's a badly named type. It simply means that the column will return datetimes in the client's timezone, not that the column contains timezone information. In fact it's the timestamp without time zone column that can be safely saved and retrieved in UTC without the database messing with it.

It's also inconsistent with how the rest of Ecto / Postgrex handles that column both now and historically. If your ecto schema specifies :utc_datetime on a timestamp without time zone column it will happily let you insert and query by %DateTime{} structs. This is entirely safe because the stipulation of :utc_datetime ensures that only actual UTC times are saved. Historically the timestamp without time zone as the default column type chosen when specifying UTC datetime in migrations. This was the correct choice both then and now.

[wojtekmach]

timestamp with time zone does NOT include timezone information. It's a badly named type. It simply means that the column will return datetimes in the client's timezone, not that the column contains timezone information

Right, but I think in order to return timestamp in client timezone the only option is to put it into %DateTime{}. In practice we only allow UTC anyway [1] but I assume the idea was to eventually support different client’s timezones.

Btw Im not arguing against what you’re saying, just showing my understanding (which might be wrong!)

[1] https://github.com/elixir-ecto/postgrex/blob/v0.14.0-rc.1/lib/postgrex/extensions/timestamptz.ex#L41

[benwilson512]

@wojtekmach that makes sense, I definitely get why values that come from a timestamp with timezone column return as %DateTime{}.

I guess what I'm not clear on right now is whether the 0.14 behaviour is considered a bug / problem, or whether the aim is for this to intentionally be the new normal.

[josevalim]

Right, timestampz does not include the timezone but we allegedly know the timezone based on the connection (and postgrex requires it to always be UTC). We could however allow datetimes in timestamp (without zone), because downcasts are usually ok.

[benwilson512]

Side note: In re-reading my posts I sound sort of annoyed, so I apologize for that. Ecto 3.0 and Postgrex 0.14 looks like a ton of work and I sincerely appreciate what everyone has done.

Downcasting would be really nice, particularly in this case where all the ordinary Ecto functions seem to already be doing that happily.

[josevalim]

I have talked to @ericmj and we agreed on downcasting Datetime to NaiveDatetime (ignoring the offset fields, which is what Elixir does) for timestamp without zone exclusively. This should be relatively straight-forward to implement and if anyone wants to submit a PR it is very appreciated. 👍