postgres: Create PgInterval type

[dimtion]

This PR creates a new type binding for PostgreSQL INTERVAL type.

Fixes #197

Some notes:

• The PR exposes a new type: sqlx::postgres::types::PgInterval which is fully compatible with PG INTERVAL type
• I did not implemented to support for https://crates.io/crates/pg_interval since the project does not seems to be actively maintained.
• I've implemented TryFrom for chrono::Duration and time::Duration, theses conversions fail if there is an overflow, if using incompatible fields (PG days and month fields) or if there is loss of precision by using nanoseconds.

[mehcode]

Can you add some tests to postgres-types ?

[mehcode]

I'm reasonably sure that you want to impl TryFrom<PgInterval> for Duration instead.

[mehcode]

If we are making the fields of PgInterval public, there isn't much point to have the new here.

[mehcode]

The chrono and time files are already pretty big. This impls might be better in interval.rs with the appropriate cfg flags. Just a minor thought.

[abonander]

theses conversions fail [...] if there is loss of precision by using nanoseconds.

This may actually cause a lot of errors if people are inserting Durations based on precise timers. Maybe instead of/in addition to a TryFrom<Duration> for PgInterval we need to have like a PgInterval::truncate_nanos(Duration) constructor or something that doesn't issue this error (but still covers the other error cases).

[dimtion]

That is a good idea, I think that replacing the TryFrom behavior by silencing this type of issue could generate even more bugs that go unnoticed. So I would think that, as you said, adding a new constructor would be better. And I should add a bit of documentation to highlight the difference between the two.

I have one question however, I not that seasoned in Rust and I'm not sure how to create a function PgInterval::truncate_nanos(Duration) that could either take a chrono::Duration or a time::Duration, do you have any idea how I should do that?

Thank you

[dimtion]

I've created two functions chrono_truncate_nanos and time_truncate_nanos in bd27d0e.

I'm not sure if it is the right thing to do. Please let me know how I can improve that.

[abonander]

@dimtion Sorry for the delay, would you mind providing impls for std::time::Duration as well? It seems fitting.

As for truncate_nanos, there's a few options I see:

• only define the constructor for std::time::Duration and expect the user to convert their chrono::Duration or time::Duration first
• define a trait TruncateNanos and implement it for all of [chrono, std::time, time]::Duration, then have the constructor take impl TruncateNanos
• keep the separate constructors but add the crate name as a postfix, i.e. truncate_nanos_chrono, truncate_nanos_time, truncate_nanos_std

[jhpratt]

I don't think this impl is a good idea. As the Interval type is a proper interval and not just a duration, attempting to treat a Duration as such is error-prone at best. In the future, the time crate will provide for a Period type that could be properly mapped to the Interval appropriately. There's no timeframe on this, but I thought it would be best to make my thoughts known before this potentially lands.

[dimtion]

@jhpratt what you says makes a lot of sense, and is indeed the core of the issue I faced. An interval/period is not the same duration, and converting between the two must not be an implicit thing.

@abonander do you think creating a PR only providing the PgInterval type could be enough? This would allow to make sqlx work with PostgreSQL Interval type, something is impossible right now. All the other type compatibility are only here to be handy in the end, and could be postponed if deemed error prone.

[jhpratt]

To be clear, I don't think converting a Period to a Duration directly will ever be allowed in the time crate. I'll almost certainly implement From<Duration> for Period when the latter is created, but only because some number of seconds would be a strict subset of what is storable in a period type.

Obviously you're free to do as you wish, but that's my perspective from the time crate.

[mehcode]

I have to agree with @jhpratt here about the Period type and the concerns around conversion. As for this PR, I think we should accept a minimal useful surface for now.

• A PgInterval type that is explicitly those three fields that fully supports the protocol and has Type + Encode + Decode.

• Type + Encode for time::Duration, chrono::Duration, and std::time::Duration where Encode fails if we cannot express the duration as a PgInterval.

• (in a later PR) Type + Encode + Decode for time::Period when that type is available.

[dimtion]

Ok, so I've rebased this branch with master:

• Commit 1: creates PgInterval type that fully supports Type + Encode + Decode
• Commit 2: adds a compatibility layer for std::time::Duration, time::Duration & chrono::Duration. It implements TryFrom<Duration> for PgInterval, and fails if there is an overflow or if there is loss of precision
• I've implemented Type + Encode for time::Duration, chrono::Duration, and std::time::Duration using this TryFrom
• Following @abonander idea, I did not remove the truncate_nanos_* methods since they might be useful in some cases.

Let me know if this implementation is better.

[jhpratt]

Looking at the overall diff, it looks good to me. I just checked the bits relevant to the time crate, though.

One but, I believe it would be more efficient to check if the nanos % 1000 == 0 instead of performing more complicated arithmetic.

[mehcode]

@dimtion Thank you ❤️ for sticking with this. I merged it and narrowed down the public API further. We can always add methods to it if it becomes a pattern people want ( the truncate approach ) but I think we're probably best off waiting for a cleaner type like time::Period.

@jhpratt Thank you as well for reviewing this as it moved along.