Skip to content

Commit

Permalink
sql: align the behavior of 'infinity' timestamp with Postgres
Browse files Browse the repository at this point in the history
Previously, "SELECT 'infinity'::timestamp" would return a concrete
timestamp instead of "infinity", which is incompatible with
Postgres, causing issue #41564.

This commit addresses this issue by:

- Returning "infinity" for 'infinity'::timestamp, supporting both text
  and binary formats.

This commit also updates the cmp/eval behavior of "infinity" timestamps.
The updated behavior is the same as Postgres:

- "infinity" is always larger than other timestamps, and "infinity"
  equals itself.
- "-infinity" is always smaller than other timestamps, and "-infinity"
  equals itself.
- "infinity"/"-infinity" add or subtract any duration results in
  itself.

Fixes: #41564

Release note (bug fix): Fixed a bug where 'infinity'::timestamp
returned a different result than Postgres.
  • Loading branch information
XiaochenCui committed Aug 26, 2024
1 parent 81f6a82 commit 63f8a13
Show file tree
Hide file tree
Showing 17 changed files with 265 additions and 146 deletions.
19 changes: 17 additions & 2 deletions pkg/cmd/generate-binary/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,22 @@
// The target postgres server must accept plaintext (non-ssl) connections from
// the postgres:postgres account. A suitable server can be started with:
//
// `docker run -p 127.0.0.1:5432:5432 postgres:11`
// Start a postgres14 server with postgis extension:
//
// The output of this file generates pkg/sql/pgwire/testdata/encodings.json.
// docker run --name postgres \
// -e POSTGRES_DB=db \
// -e POSTGRES_HOST_AUTH_METHOD=trust \
// -p 127.0.0.1:5432:5432 \
// postgis/postgis:14-3.4
//
// docker exec -it postgres psql -U postgres -c "CREATE EXTENSION postgis;"
//
// TODO(xiaochen): figure out where the `"Text": "9E+4"` in encodings.json comes from
// and fix it. (postgres 9 ~ 14 all return "90000" for `SELECT '9E+4'::decimal;`)
//
// Generate file "encodings.json":
//
// bazel run pkg/cmd/generate-binary > pkg/sql/pgwire/testdata/encodings.json
package main

import (
Expand Down Expand Up @@ -313,6 +326,8 @@ var inputs = map[string][]string{
"0004-10-19 10:23:54 BC",
"4004-10-19 10:23:54",
"9004-10-19 10:23:54",
"infinity",
"-infinity",
},

"'%s'::timestamptz": {
Expand Down
1 change: 0 additions & 1 deletion pkg/cmd/roachtest/tests/libpq_blocklist.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ var libPQBlocklist = blocklist{
"pq.TestCopyInRaiseStmtTrigger": "5807",
"pq.TestCopyInTypes": "5807",
"pq.TestCopyRespLoopConnectionError": "5807",
"pq.TestInfinityTimestamp": "41564",
"pq.TestIssue186": "41558",
"pq.TestIssue196": "41689",
"pq.TestIssue282": "12137",
Expand Down
4 changes: 0 additions & 4 deletions pkg/cmd/roachtest/tests/pgjdbc_blocklist.go
Original file line number Diff line number Diff line change
Expand Up @@ -637,10 +637,6 @@ var pgjdbcBlockList = blocklist{
`org.postgresql.test.jdbc42.GetObject310Test.testGetOffsetTime[binary = REGULAR]`: "unknown",
`org.postgresql.test.jdbc42.Jdbc42CallableStatementTest.testGetResultSetWithoutArg`: "17511",
`org.postgresql.test.jdbc42.Jdbc42CallableStatementTest.testGetResultSetWithoutArgUnsupportedConversion`: "17511",
`org.postgresql.test.jdbc42.SetObject310InfinityTests.testTimestamp[binary = FORCE]`: "41564",
`org.postgresql.test.jdbc42.SetObject310InfinityTests.testTimestamp[binary = REGULAR]`: "41564",
`org.postgresql.test.jdbc42.SetObject310InfinityTests.testTimestamptz[binary = FORCE]`: "41564",
`org.postgresql.test.jdbc42.SetObject310InfinityTests.testTimestamptz[binary = REGULAR]`: "41564",
`org.postgresql.test.plugin.AuthenticationPluginTest.authPluginMD5()`: "unknown",
`org.postgresql.test.util.PasswordUtilTest.alterUserPasswordSupportsNullEncoding()`: "73337",
`org.postgresql.test.util.PasswordUtilTest.customScramParams()`: "unknown",
Expand Down
3 changes: 1 addition & 2 deletions pkg/sql/logictest/testdata/logic_test/datetime
Original file line number Diff line number Diff line change
Expand Up @@ -1707,11 +1707,10 @@ SET TIME ZONE 0

subtest infinity_time

# TODO(#41564): this should display "infinity", "-infinity".
query TT
SELECT 'infinity'::timestamp, '-infinity'::timestamptz
----
294276-12-31 23:59:59.999999 +0000 +0000 -4713-11-24 00:00:00 +0000 +0000
infinity -infinity

# Verify that parse_timestamp can be used in computed column expressions.
statement ok
Expand Down
100 changes: 50 additions & 50 deletions pkg/sql/logictest/testdata/logic_test/lookup_join

Large diffs are not rendered by default.

47 changes: 46 additions & 1 deletion pkg/sql/logictest/testdata/logic_test/timestamp
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ SELECT to_timestamp(1646906263.123456), to_timestamp(1646906263), to_timestamp('
query TT
SELECT to_timestamp('infinity'), to_timestamp('-infinity')
----
294276-12-31 23:59:59.999999 +0000 UTC -4713-11-24 00:00:00 +0000 UTC
infinity -infinity

## Test for to_timestamp with NULL
query T
Expand Down Expand Up @@ -783,3 +783,48 @@ SET timezone = '04:15';
SELECT to_char(now(), 'of') as of_t, to_char(now(), 'tzh:tzm') as "tzh:tzm";
----
-04:15 -04:15

# Test for issue #41564, the behavior of infinity should be consistent with PostgreSQL.
subtest infinity_41564

query T
SELECT 'infinity':::TIMESTAMP
----
infinity

query T
SELECT '-infinity':::TIMESTAMP
----
-infinity

# infinity is greater than any timestamp, including '294276-12-31 23:59:59.999999' (MaxSupportedTime)
query B
SELECT 'infinity'::timestamp > '294276-12-31 23:59:59.999999'::timestamp;
----
true

# -infinity is less than any timestamp, including '4714-11-24 00:00:00+00 BC' (MinSupportedTime)
query B
SELECT '-infinity'::timestamp < '4714-11-24 00:00:00+00 BC'::timestamp;
----
true

# infinity and -infinity should be equal to themselves
query BB
SELECT 'infinity'::timestamp = 'infinity'::timestamp, '-infinity'::timestamp = '-infinity'::timestamp;
----
true true

# infinity add/subtract any interval results in itself
query TT
SELECT 'infinity'::timestamp + '1 second'::interval, 'infinity'::timestamp - '1 second'::interval;
----
infinity infinity

# -infinity add/subtract any interval results in itself
query TT
SELECT '-infinity'::timestamp + '1 second'::interval, '-infinity'::timestamp - '1 second'::interval;
----
-infinity -infinity

subtest end
6 changes: 6 additions & 0 deletions pkg/sql/pgwire/pgwirebase/encoding.go
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,12 @@ type PGNumeric struct {
// for a timestamp. To create a timestamp from this value, it takes the microseconds
// delta and adds it to PGEpochJDate.
func pgBinaryToTime(i int64) time.Time {
if i == math.MaxInt64 {
return pgdate.TimeInfinity
}
if i == math.MinInt64 {
return pgdate.TimeNegativeInfinity
}
return duration.AddMicros(PGEpochJDate, i)
}

Expand Down
14 changes: 14 additions & 0 deletions pkg/sql/pgwire/testdata/encodings.json
Original file line number Diff line number Diff line change
Expand Up @@ -1980,6 +1980,20 @@
"TextAsBinary": [57, 48, 48, 52, 45, 49, 48, 45, 49, 57, 32, 49, 48, 58, 50, 51, 58, 53, 52],
"Binary": [3, 17, 83, 233, 31, 54, 66, 128]
},
{
"SQL": "'infinity'::timestamp",
"Oid": 1114,
"Text": "infinity",
"TextAsBinary": [105, 110, 102, 105, 110, 105, 116, 121],
"Binary": [127, 255, 255, 255, 255, 255, 255, 255]
},
{
"SQL": "'-infinity'::timestamp",
"Oid": 1114,
"Text": "-infinity",
"TextAsBinary": [45, 105, 110, 102, 105, 110, 105, 116, 121],
"Binary": [128, 0, 0, 0, 0, 0, 0, 0]
},
{
"SQL": "'1999-01-08 04:05:06+00'::timestamptz",
"Oid": 1184,
Expand Down
11 changes: 11 additions & 0 deletions pkg/sql/pgwire/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,17 @@ func (b *writeBuffer) writeBinaryColumnarElement(
// is represented as the number of microseconds between the given time and Jan 1, 2000
// (dubbed the PGEpochJDate), stored within an int64.
func timeToPgBinary(t time.Time, offset *time.Location) int64 {
if t == pgdate.TimeInfinity {
// Postgres uses math.MaxInt64 microseconds as the infinity value.
// See: https://github.com/postgres/postgres/blob/42aa1f0ab321fd43cbfdd875dd9e13940b485900/src/include/datatype/timestamp.h#L107.
return math.MaxInt64
}
if t == pgdate.TimeNegativeInfinity {
// Postgres uses math.MinInt64 microseconds as the negative infinity value.
// See: https://github.com/postgres/postgres/blob/42aa1f0ab321fd43cbfdd875dd9e13940b485900/src/include/datatype/timestamp.h#L107.
return math.MinInt64
}

if offset != nil {
t = t.In(offset)
} else {
Expand Down
19 changes: 14 additions & 5 deletions pkg/sql/sem/tree/datum.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,14 @@ var (
// postgres 4714 BC (JULIAN = 0) - 4713 in their docs - and 294276 AD.

// MaxSupportedTime is the maximum time we support parsing.
//
// Refer to the doc comments of the function "timeutil.Unix" for the process of
// deriving the arguments to construct a specific time.Time.
MaxSupportedTime = timeutil.Unix(9224318016000-1, 999999000) // 294276-12-31 23:59:59.999999
// MinSupportedTime is the minimum time we support parsing.
//
// Refer to the doc comments of the function "timeutil.Unix" for the process of
// deriving the arguments to construct a specific time.Time.
MinSupportedTime = timeutil.Unix(-210866803200, 0) // 4714-11-24 00:00:00+00 BC
)

Expand Down Expand Up @@ -2592,12 +2598,11 @@ type DTimestamp struct {
}

// MakeDTimestamp creates a DTimestamp with specified precision.
func MakeDTimestamp(t time.Time, precision time.Duration) (*DTimestamp, error) {
ret := t.Round(precision)
if ret.After(MaxSupportedTime) || ret.Before(MinSupportedTime) {
return nil, NewTimestampExceedsBoundsError(ret)
func MakeDTimestamp(t time.Time, precision time.Duration) (_ *DTimestamp, err error) {
if t, err = checkTimeBounds(t, precision); err != nil {
return nil, err
}
return &DTimestamp{Time: ret}, nil
return &DTimestamp{Time: t}, nil
}

// MustMakeDTimestamp wraps MakeDTimestamp but panics if there is an error.
Expand Down Expand Up @@ -2871,6 +2876,10 @@ type DTimestampTZ struct {
func checkTimeBounds(t time.Time, precision time.Duration) (time.Time, error) {
ret := t.Round(precision)
if ret.After(MaxSupportedTime) || ret.Before(MinSupportedTime) {
if t == pgdate.TimeInfinity || t == pgdate.TimeNegativeInfinity {
return t, nil
}

return time.Time{}, NewTimestampExceedsBoundsError(ret)
}
return ret, nil
Expand Down
8 changes: 8 additions & 0 deletions pkg/sql/sem/tree/pgwire_encode.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/timeofday"
"github.com/cockroachdb/cockroach/pkg/util/timetz"
"github.com/cockroachdb/cockroach/pkg/util/timeutil/pgdate"
"github.com/lib/pq/oid"
)

Expand Down Expand Up @@ -334,6 +335,13 @@ func PGWireFormatTimeTZ(t timetz.TimeTZ, tmp []byte) []byte {
// PGWireFormatTimestamp formats t into a format lib/pq understands.
// If offset is not nil, it will not display the timezone offset.
func PGWireFormatTimestamp(t time.Time, offset *time.Location, tmp []byte) (b []byte) {
if t == pgdate.TimeInfinity {
return []byte("infinity")
}
if t == pgdate.TimeNegativeInfinity {
return []byte("-infinity")
}

format := PGTimeStampFormatNoOffset
if offset != nil {
format = PGTimeStampFormat
Expand Down
44 changes: 20 additions & 24 deletions pkg/sql/stats/quantile.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,10 +358,11 @@ func isValidCount(x float64) bool {

// toQuantileValue converts from a datum to a float suitable for use in a quantile
// function. It differs from eval.PerformCast in a few ways:
// 1. It supports conversions that are not legal casts (e.g. DATE to FLOAT).
// 2. It errors on NaN and infinite values because they will break our model.
// fromQuantileValue is the inverse of this function, and together they should
// support round-trip conversions.
// 1. It supports conversions that are not legal casts (e.g. DATE to FLOAT).
// 2. It errors on NaN and infinite values because they will break our model.
// fromQuantileValue is the inverse of this function, and together they should
// support round-trip conversions.
//
// TODO(michae2): Add support for DECIMAL, TIME, TIMETZ, and INTERVAL.
func toQuantileValue(d tree.Datum) (float64, error) {
switch v := d.(type) {
Expand All @@ -380,32 +381,27 @@ func toQuantileValue(d tree.Datum) (float64, error) {
// converting back.
return float64(v.PGEpochDays()), nil
case *tree.DTimestamp:
if v.Equal(pgdate.TimeInfinity) || v.Equal(pgdate.TimeNegativeInfinity) {
return 0, tree.ErrFloatOutOfRange
if v.Equal(pgdate.TimeInfinity) {
return pgdate.TimeInfinitySec, nil
}
if v.Equal(pgdate.TimeNegativeInfinity) {
return pgdate.TimeNegativeInfinitySec, nil
}
return float64(v.Unix()) + float64(v.Nanosecond())*1e-9, nil
case *tree.DTimestampTZ:
// TIMESTAMPTZ doesn't store a timezone, so this is the same as TIMESTAMP.
if v.Equal(pgdate.TimeInfinity) || v.Equal(pgdate.TimeNegativeInfinity) {
return 0, tree.ErrFloatOutOfRange
if v.Equal(pgdate.TimeInfinity) {
return pgdate.TimeInfinitySec, nil
}
if v.Equal(pgdate.TimeNegativeInfinity) {
return pgdate.TimeNegativeInfinitySec, nil
}
return float64(v.Unix()) + float64(v.Nanosecond())*1e-9, nil
default:
return 0, errors.Errorf("cannot make quantile value from %v", d)
}
}

var (
// quantileMinTimestamp is an alternative minimum finite DTimestamp value to
// avoid the problems around TimeNegativeInfinity, see #41564.
quantileMinTimestamp = tree.MinSupportedTime.Add(time.Second)
quantileMinTimestampSec = float64(quantileMinTimestamp.Unix())
// quantileMaxTimestamp is an alternative maximum finite DTimestamp value to
// avoid the problems around TimeInfinity, see #41564.
quantileMaxTimestamp = tree.MaxSupportedTime.Add(-1 * time.Second).Truncate(time.Second)
quantileMaxTimestampSec = float64(quantileMaxTimestamp.Unix())
)

// fromQuantileValue converts from a quantile value back to a datum suitable for
// use in a histogram. It is the inverse of toQuantileValue. It differs from
// eval.PerformCast in a few ways:
Expand Down Expand Up @@ -471,11 +467,11 @@ func fromQuantileValue(colType *types.T, val float64) (tree.Datum, error) {
case types.TimestampFamily, types.TimestampTZFamily:
sec, frac := math.Modf(val)
var t time.Time
// Clamp to (our alternative finite) DTimestamp bounds.
if sec <= quantileMinTimestampSec {
t = quantileMinTimestamp
} else if sec >= quantileMaxTimestampSec {
t = quantileMaxTimestamp
// Clamp to DTimestamp bounds.
if sec <= pgdate.TimeNegativeInfinitySec {
t = pgdate.TimeNegativeInfinity
} else if sec >= pgdate.TimeInfinitySec {
t = pgdate.TimeInfinity
} else {
t = timeutil.Unix(int64(sec), int64(frac*1e9))
}
Expand Down
Loading

0 comments on commit 63f8a13

Please sign in to comment.