Bad conversion from non-UTC timestamptz with postgres

[Nemo157]

This comment here is incorrect:

sqlx/sqlx-core/src/postgres/types/chrono/datetime.rs

Lines 79 to 81 in 2e1658e

	// Contains a time-zone specifier
	// This is given for timestamptz for some reason
	// Postgres already guarantees this to always be UTC

If you have a connection set to use a non-UTC timezone you will get a response with timestamps in that timezone. After overriding the executor to use PgValueFormat::Text for responses and adding some extra debugging code, running the following code:

let date = Utc.ymd(2020, 1, 1).and_hms(1, 1, 1);
let mut conn = pool.acquire().await?;
sqlx::query("SET TIME ZONE 'Europe/Berlin'")
    .fetch_optional(&mut conn).await?;
let row: (DateTime<Utc>,) = sqlx::query_as("SELECT $1::timestamptz")
    .bind(&date)
    .fetch_one(&mut conn).await?;

assert_eq!(row.0, date);

The assertion failed because the timezone on the response was not taken into account

[/tmp/tmp.ErEoBwofoQ/sqlx-core-0.4.0-beta.1/src/postgres/types/chrono/datetime.rs:79] ("decode", s) = (
    "decode", "2020-01-01 02:01:01+01",
)
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `2020-01-01T02:01:01Z`,
 right: `2020-01-01T01:01:01Z`', src/main.rs:19:5

---

I can't see any way to trigger this behaviour from outside sqlx-core since it always uses the binary format for responses, but it seems like a potential footgun to leave around in case this does someday become configurable.

[abonander]

This is possible to trigger if you use Executor::query() with a query string instead of sqlx::query(), et al (or the macros), as that uses the singular Query command which gets responses in text format instead of Parse/Bind/Execute.

We should be parsing DateTime using DateTime::parse_from_rfc_3339 which handles the timezone specifier correctly, not forwarding to NaiveDateTime.

[yuyawk]

ANNOTATION ADDED BY @yuyawk HIM/HERSELF: In this comment s/he misunderstood what this issue mean. See also an advice to him/her

It seems that this issue is not reproduced at the current HEAD (perhaps already fixed?).

I added the test below to tests/postgres/postgres.rs, set up docker-compose and carried out ./tests/x.py --target postgres, but the test passed.

test that I added:

#[sqlx_macros::test]
async fn test_issue_703() -> anyhow::Result<()> {
    use sqlx::types::chrono::{DateTime, TimeZone, Utc};
    let mut conn = new::<Postgres>().await?;
    let date = Utc.ymd(2020, 1, 1).and_hms(1, 1, 1);
    sqlx::query("SET TIME ZONE 'Europe/Berlin'")
        .fetch_optional(&mut conn)
        .await?;
    let row: (DateTime<Utc>,) = sqlx::query_as("SELECT $1::timestamptz")
        .bind(&date)
        .fetch_one(&mut conn)
        .await?;
    assert_eq!(row.0, date);

    Ok(())
}

P.S.

As well as the original issue, it might be better to use PoolConnection. The modified test below is also passable.

#[sqlx_macros::test]
async fn test_issue_703() -> anyhow::Result<()> {
    use sqlx::types::chrono::{DateTime, TimeZone, Utc};
    use sqlx_test::pool;
    let pool = pool::<Postgres>().await?;
    let mut conn = pool.acquire().await?;
    let date = Utc.ymd(2020, 1, 1).and_hms(1, 1, 1);
    sqlx::query("SET TIME ZONE 'Europe/Berlin'")
        .fetch_optional(&mut conn)
        .await?;
    let row: (DateTime<Utc>,) = sqlx::query_as("SELECT $1::timestamptz")
        .bind(&date)
        .fetch_one(&mut conn)
        .await?;
    assert_eq!(row.0, date);

    Ok(())
}

[Nemo157]

As mentioned this doesn't occur normally because the binary result format is used. I managed to trigger it without editing sqlx-core to override that by using the direct text query as @abonander said:

    let row: (DateTime<Utc>,) = sqlx::query_as("SELECT '2020-01-01 01:01:01'::timestamptz")
                .fetch_one(&mut conn).await?;

But weirdly, if I also do this query before the SET TIME ZONE query, it causes it to not fail when queried afterwards.

EDIT: Adding in some more debugging shows that this is still returning a Binary response, what you actually need to test:

    use sqlx::{Row, Executor};
    let row = conn.fetch_one("SELECT '2020-01-01 01:01:01+00'::timestamptz").await?;
    assert_eq!(row.get::<DateTime<Utc>, _>(0), date);

[yuyawk]

@Nemo157 Thank you for your explanation. I totally misunderstood what this issue means. Sorry to trouble you.
I reproduced that test failure.

[yuyawk]

I created a PR to fix this issue, but I'm not so confident about how to convert the output into types with timezone not determined on the application side, i.e. NaiveDateTime and DateTime<FixedOffset>.

If my understanding is correct:

• For NaiveDateTime, the existing implementation implies that the timezone the output depends on differs according to the value of PgValueFormat.
• For DateTime<FixedOffset>, the existing implementation implies that the timezone of the output follows the local time of the server where the application is running.

In my PR, I kept these behaviors unchanged, but in my opinion, it may be natural that these values are determined according to the timezone of the database.

[yuyawk]

My PR #1484 is waiting for reviewers to approve it

quoted:

1 workflow awaiting approval
First-time contributors need a maintainer to approve running workflows

[Flowneee]

@abonander Hi. I am currently looking at this problem and interested in reviving PR above or make a new one.

There is also problem that I cannot use custom timezones, because decode is implemented only for Utc, Local and FixedOffset. I think something like that would be more appropriate

impl<'r, Tz: Timezone> Decode<'r, Postgres> for DateTime<Tz> {
    fn decode(value: PgValueRef<'r>) -> Result<Self, BoxDynError> {
        ...
    }
}

I'll try to come up with solution if that's ok.

[pierre-wehbe]

@Flowneee Happy to pair on this as I am also facing the same problem.

There is an associated problem that was fixed for SQLite here
Ideally the code below should be modified to use DateTime::parse_from_rfc3339(value) when the date is available as TEXT.

My expectations are, if I store a date: 2024-01-01T00:00:00-07:00, I expect the decoded value to be the same 2024-01-01T00:00:00-07:00 and NOT 2023-12-31T17:00:00-00:00

impl<'r> Decode<'r, Postgres> for DateTime<FixedOffset> {
    fn decode(value: PgValueRef<'r>) -> Result<Self, BoxDynError> {
        let naive = <NaiveDateTime as Decode<Postgres>>::decode(value)?;
        Ok(Utc.fix().from_utc_datetime(&naive))
    }
}

@abonander Can you provide guidance on whether we're on the right track?

Also related code that might need to be modified: if s.contains('+') feels flaky since it could also be '-'

PgValueFormat::Text => {
                let s = value.as_str()?;
                NaiveDateTime::parse_from_str(
                    s,
                    if s.contains('+') {
                        // Contains a time-zone specifier
                        // This is given for timestamptz for some reason
                        // Postgres already guarantees this to always be UTC
                        "%Y-%m-%d %H:%M:%S%.f%#z"
                    } else {
                        "%Y-%m-%d %H:%M:%S%.f"
                    },
                )?
            }

[Nemo157]

My expectations are, if I store a date: 2024-01-01T00:00:00-07:00, I expect the decoded value to be the same 2024-01-01T00:00:00-07:00 and NOT 2023-12-31T17:00:00-00:00

Note that Postgres does not store timezones. The timestamptz type is purely a convenience type to automatically convert input timestamps to UTC and output timestamps to the connection's timezone. (Personally I see no reason to use it in modern applications and think all timezone handling should be left to application side, but all the rust crates have decided that timestamp is naïve, not UTC).

[pierre-wehbe]

@Nemo157 not sure what you mean, there is a specific type called "timestamp with timezone".
I opened a PR that solves it: #3411

Waiting for tests to pass.

My expectations are, if I store a date: 2024-01-01T00:00:00-07:00, I expect the decoded value to be the same 2024-01-01T00:00:00-07:00 and NOT 2023-12-31T17:00:00-00:00

Note that Postgres does not store timezones. The timestamptz type is purely a convenience type to automatically convert input timestamps to UTC and output timestamps to the connection's timezone. (Personally I see no reason to use it in modern applications and think all timezone handling should be left to application side, but all the rust crates have decided that timestamp is naïve, not UTC).

[Nemo157]

All timezone-aware dates and times are stored internally in UTC. They are converted to local time in the zone specified by the TimeZone configuration parameter before being displayed to the client.
—https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-TIMEZONES

Try running SET TIME ZONE 'Europe/Berlin' before doing your query, you'll see it's not returned in -07 anymore, or try storing a timestamp in a timezone other than -07 and you'll see it returned in -07.

[pierre-wehbe]

@Nemo157 Thanks! Just double checked, sad since SQLite does store all of it as text so it can be queried back..

[tisonkun]

Also related code that might need to be modified: if s.contains('+') feels flaky since it could also be '-'

.contains('-') is always true because it's contained in %Y-%m-%d. I'm trying to support similar functionality in #3511. You may take a look.