Support BC form of timestamps and dates.

[condorcet]

Hello! I faced some problem with textual representation of dates and timestamps before common era (BC). It is actual only in simple protocol but not in binary.

Firstly if we have a "negative" date in Go or in other words t.Before(time.Time{}) == true then textual representation of this date in Postgres must contain BC suffix. Postgres supports dates before common era, but naive string representation of time.Time is not fit for this purposes. For example minute before zero time will be in Go 0000-12-31 23:59:00 but for Postgres expects 0001-12-31 23:59:00 BC.

Secondly I've noticed that even after required improvements were added in pgtype it doesn't took an effect. Because date types in pgtype returning time.Time "as is" here https://github.com/condorcet/pgtype/commit/affe9e2dc7ae59a9c01f2fe857bdaee160859ca3#diff-1aef962e6259426c493d2499a5f85405d51a760644acdc53a9e51e0fbbf7b5ccL235 Instead of serialization. For example if we use pgtype.Timestamp in application code then appropriate query/exec argument has driver.Valuer type https://github.com/jackc/pgx/blob/master/values.go#L65 but after that we again get native time.Time without proper serialization. So I made explicit encoding in Value methods for date types. Is it ok?

Lastly I think we can use new helpers for encoding/decoding right into pgx https://github.com/jackc/pgx/blob/38dd42de4bc5ba4b7492b3a07ae4e472fab9517d/values.go#L82 for more user friendly experience with native time.Time. What do you think about it?

[jackc]

Mostly looks good, but I'm getting a test failure on my machine.

--- FAIL: TestTimestamptzTranscode (0.02s)
    testutil.go:159: Param TextFormat Result TextFormat 10: expected {0001-01-01 00:00:00 +0000 UTC 2 none}, got {0000-12-31 18:09:24 +0000 UTC 2 none}
    testutil.go:159: Param BinaryFormat Result TextFormat 10: expected {0001-01-01 00:00:00 +0000 UTC 2 none}, got {0000-12-31 18:09:24 +0000 UTC 2 none}
    testutil.go:159: Param TextFormat Result TextFormat 11: expected {0000-01-01 00:00:00 -0550 LMT 2 none}, got {0000-01-01 00:00:00 +0000 UTC 2 none}
    testutil.go:159: Param BinaryFormat Result TextFormat 11: expected {0000-01-01 00:00:00 -0550 LMT 2 none}, got {0000-01-01 00:00:00 +0000 UTC 2 none}
    testutil.go:159: Param TextFormat Result TextFormat 12: expected {-0100-01-01 00:00:00 -0550 LMT 2 none}, got {-0100-01-01 00:00:00 +0000 UTC 2 none}
    testutil.go:159: Param BinaryFormat Result TextFormat 12: expected {-0100-01-01 00:00:00 -0550 LMT 2 none}, got {-0100-01-01 00:00:00 +0000 UTC 2 none}

Also, I don't understand the change to Value to return a string instead of a time.Time. I changed it back and no tests failed, so I don't understand exactly what that is doing. If it's necessary can we add a test to clarify it?

As far as exporting DecodeTextTimestamp and EncodeTextTimestamp, my expectation is that consumers of this new BC text mode support would be entirely through existing interfaces.

[condorcet]

Mostly looks good, but I'm getting a test failure on my machine.

Hm. Thank you I'll try to investigate it.

As far as exporting DecodeTextTimestamp and EncodeTextTimestamp, my expectation is that consumers of this new BC text mode support would be entirely through existing interfaces.

I mean that we could use this helpers under the hood of pgx here https://github.com/jackc/pgx/blob/master/internal/sanitize/sanitize.go#L51 And for end users time will be encoded implicitly.

But OK, I'll make this methods private for now.

Also, I don't understand the change to Value to return a string instead of a time.Time

In app code with pgx even if we wrap native time with right type we don't get expected result. Because pgx handles Valuer interface before pgtype.TextEncoder when using simple protocol (https://github.com/jackc/pgx/blob/master/values.go#L65)

I've made simple test:

...
t1 := time.Time{}.Add(-1*time.Minute) // BC
t2 := pgtype.Timestamp{} // using type that support BC
t2.Set(t1)
var res2 pgtype.Timestamp
err = conn.QueryRow(context.Background(), "SELECT $1::timestamp", t2).Scan(&res2)
require.NoError(t, err)

There will be out of range error from postgres:

Error:          Received unexpected error:
                                ERROR: date/time field value out of range: "0000-12-31 23:59:00Z" (SQLSTATE 22008)
                Test:           Test_XXX

because pgx takes pgtype.Timestamp -> checks if it implementing Valuer interface -> invokes Value() method that returns time.Time -> get native time string representation which incompatible with postgres timestamp format.

But we can avoid the problem if Value method returns not native time but correctly encoded timestamp string.

[jackc]

Hmm... That simple protocol path is tricky... But at the very least it is surprising for Value() to return a string when time.Time is a valid driver.Value (and it could be considered a mild compatibility break). It also makes me wonder what happens when using a BC time.Time directly with the simple protocol -- both directly with pgx and through database/sql.

Would it make sense the pgx simple protocol path to handle time.Time specially? I think that would handle direct uses of time.Time as well as remove the need to change the return type of Value().

[condorcet]

Would it make sense the pgx simple protocol path to handle time.Time specially?

Do you mean a special case like JSONB here? https://github.com/jackc/pgx/blob/master/values.go#L46 Well, I think it is a fair trade-off between potential breaking changes of pgtype date types and supporting rare BC case :)

[jackc]

Do you mean a special case like JSONB here?

Yes.

I'm still getting a strange test failure. It looks like decoding the text format is not handling time zones properly.

--- FAIL: TestTimestamptzTranscode (0.02s)
    testutil.go:159: Param TextFormat Result TextFormat 10: expected {0001-01-01 00:00:00 +0000 UTC 2 none}, got {0000-12-31 18:09:24 +0000 UTC 2 none}
    testutil.go:159: Param BinaryFormat Result TextFormat 10: expected {0001-01-01 00:00:00 +0000 UTC 2 none}, got {0000-12-31 18:09:24 +0000 UTC 2 none}
    testutil.go:159: Param TextFormat Result TextFormat 11: expected {0000-01-01 00:00:00 -0550 LMT 2 none}, got {0000-01-01 00:00:00 +0000 UTC 2 none}
    testutil.go:159: Param BinaryFormat Result TextFormat 11: expected {0000-01-01 00:00:00 -0550 LMT 2 none}, got {0000-01-01 00:00:00 +0000 UTC 2 none}
    testutil.go:159: Param TextFormat Result TextFormat 12: expected {-0100-01-01 00:00:00 -0550 LMT 2 none}, got {-0100-01-01 00:00:00 +0000 UTC 2 none}
    testutil.go:159: Param BinaryFormat Result TextFormat 12: expected {-0100-01-01 00:00:00 -0550 LMT 2 none}, got {-0100-01-01 00:00:00 +0000 UTC 2 none}

[condorcet]

Thank you, I've improved test to catch the problem and fixed it.

[jackc]

Sorry to keep nitpicking, but this latest change will permanently alter the test database when run, and this also means it requires superuser permissions. I'd prefer to avoid that.

I realize the test helper interface does not make it possible / easy to set the connection time zone, but if we were able to do that would that be sufficient for the test case? It might make sense for this test to not use the standard helpers.