sql: boundary dates are broken

[benesch]

> select (-9223372036854775808):::int64::date;
+--------------------------------+
| (-9223372036854775808):::INT64::DATE |
+--------------------------------+
| 1970-01-01 00:00:00+00:00      |
+--------------------------------+

root@:26257/> select (9223372036854775807):::int64::date;
+--------------------------------+
| (9223372036854775807):::INT64::DATE |
+--------------------------------+
| 1969-12-31 00:00:00+00:00      |
+--------------------------------+
(1 row)

Something somewhere is overflowing. Assigning to @knz for triage.

[knz]

A data point:

> select (9223372036854775807):::int64::date
+--------------------------------+
| (9223372036854775807):::INT64::DATE::INT |
+--------------------------------+
|            9223372036854775807 |
+--------------------------------+

So the value is properly there. It's the conversion during printing that fails.

[knz]

Okay so the proximate cause of the problem is that many computations involving DDate first convert the date (number of days since unix epoch) to a time.Time (number of seconds since epoch), by multiplying by 86400. For any value larger than MaxInt64/86400 or lower than MinInt64/86400, an overflow occurs and the result is incorrect.

This multiplication currently happens at least in the following places:

• (*DDate).Format() - probably doesn't affect clients but does affect debugging
• (*CastExpr).Eval(), when casting from DDate to DTimestamp
• built-in experimental_strftime when operating on DDate
• tree.MakeDTimestampTZFromDate(), in turn used in:
  • built-ins extract, date_trunc,
  • (*CastExpr).Eval(), when casting from DDate to DTimestampTZ
  • binop date +/- time/timestamp/timestamptz and swapped operands
  • binop date +/- interval and swapped operands
  • tree.timeFromDatum() / compareTimestamps, in turn used in:
    • (*DDate).Compare() when comparing against Timestamp/TimestampTZ, used in index selection
    • (*DTimestamp{TZ,}).Compare() when comparing against DDate, using in index selection
• pgwire/types.go: to send a date to the client

Now, the last item in this list reveals the thing that will hurt the most: pgwire communicates dates back and forth with the client as 1) 32-bit values (!) 2) as a number of seconds since the epoch, not days (!!) (edit: I was wrong)

This effectively limits the range of possible dates in pgwire to 1902-2038 (!!!).

So what are we going to do about this?

First of all I am comfortable supporting a larger range of values inside the DB than can be conveyed over pgwire. Users can be informed of this protocol limitation and work around it by converting their dates to other things (e.g. strings) when sending/extracting their data.

That said, even if we continue to use "number of days since epoch as int64" we must do something about the various conversions to "number of seconds", identified above, that are problematic because of the overflow. There are a few ways forward:

A) either allow the range of DATE to be larger than that supported by TIMESTAMP/INTERVAL. If we do this, we must error out every time a large date is casted/converted to the other types, and special case large date comparisons with the other types. This will add overhead to all the methods identified above.

B) or, refuse to let DATE values to exceed the range supported by TIMESTAMP/INTERVAL. If we do this, two separate concerns arise:

• we must refuse such large values on input and refuse to store them in the database
• we may need to deal with the possibility that such a large value already exists in the DB and deal with this when it presents itself. This in turn mandates checks in most of the methods identified above, which will also add overhead.

C) accept pg's choice for implementing DATE and restrict it to a 32-bit number of seconds since the epoch (and thus accept pg's range limitation). We'd then refuse out-of-range values on entry, and refuse out-of-range values when reading existing dates from KV. This would effectively cripple this data type and we would subsequently highly discourage users from using it in docs.

In an ideal world, we would have identified this earlier, prevent entry of large dates, and avoid adding the overhead to the various methods entirely. Then solution B above would clearly be superior.

I am inclined to decide to ignore databases that already contain large dates, and fix the problem by adding the assertion on input (and casting from int to date) without adding the checks everywhere else - i.e. solution B without the extra costs.

The rationale is that I would be surprised if any user would already have developed a use case for such large date values, so if such values slipped through they are likely mistakes in need to be corrected. If that assumption is wrong, changing the code as described above would merely cause any existing values in the database to become unusable -- they can still be edited / replaced.

Meanwhile I'm not religious on this and adopting solution A with suitable rationale would be OK with me. From a code change perspective it's more expensive however.

cc @bdarnell can you advise what you think is wisest?

[knz]

@justinj and @solongordon, I would like to suggest that you sit and brainstorm together what you personally think is the best approach here. I am thinking of you both because you were "paged in" on these issues recently. Perhaps having a casual look at what other db engines do could inform your discussion.

[knz]

Three more things:

• my analysis above of what pgwire is doing ("32-bit number of seconds") was wrong. Actually it prints out dates as a string, with an optional "BC" suffix after the year as needed.
• postgres actualy supports dates from 4713 BC to 5874897 AD by using an int32 value since some epoch which 1) doesn't seem to be the unix epoch 2) I'm not sure what the unit is
• our pgwire code to send date values to the client might be wrong - I don't see that we test it with any BC dates, and I fear that using time.Time as intermediate type restricts us to a smaller range than that supported by pg (?)

[bdarnell]

I agree with restricting the range of DATE to match that of TIMESTAMP (although it's worth noting that postgres doesn't do this; their DATE type works further into the future than their TIMESTAMP type). And I'm OK with making this change without doing anything about existing large dates that may have been written.

[solongordon]

Aligning the accepted DATE and TIMESTAMP ranges sounds good to me. However TIMESTAMP itself is not perfect:

> select (-9223372036854775808):::int64::timestamp;
+-------------------------------------------+
| (-9223372036854775808):::INT64::TIMESTAMP |
+-------------------------------------------+
| 292277026596-12-04 15:30:08+00:00         |
+-------------------------------------------+

This behavior actually goes all the way back to Go's time.Unix function, which doesn't appear to guarantee good behavior for large negative inputs.

I would propose being more restrictive with our minimum supported timestamp/date. Matching the Postgres minimum of 4713 BC seems reasonable to me since it is the beginning of the Gregorian calendar. According to their docs this follows the SQL standard: https://www.postgresql.org/docs/current/static/datetime-units-history.html

[sploiselle]

@knz Can we get a docs description this issue as a Known Limitation for 2.1?

[knz]

I think this is an experimental feature we do not otherwise document. Please check: do we have any doc about converting values of type INT(EGER) to TIME/DATE/TIMESTAMP?

If not you can disregard this issue. Otherwise:

"""
title: Conversion of integers to date/time values

CockroachDB supports an experimental extension to the SQL standard where an integer value can be converted to a DATE/TIME/TIMESTAMP value, taking the number as a number of seconds since the Unix epoch. This conversion is currently only well defined for a small range of integers and values that a large in magnitude will cause conversion errors.
"""

[sploiselle]

@knz It looks like there's a slight typo in this; can you clarify?

This conversion is currently only well defined for a small range of integers and values that a large in magnitude will cause conversion errors.

Looks like there's a word missing within a large in magnitude?

[knz]

values that a large in magnitude

yes sorry the correct phrasing is

"This conversion is currently only well defined for a small range of integers, and values that are large in magnitude (alt phrasing: large in absolute values) will cause conversion errors."

[jordanlewis]

@mjibson ping

[maddyblue]

Fixed by #36938