Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: make TIMETZ more pg-compatible #26097

Closed
knz opened this issue May 25, 2018 · 5 comments · Fixed by #43023
Closed

sql: make TIMETZ more pg-compatible #26097

knz opened this issue May 25, 2018 · 5 comments · Fixed by #43023
Assignees
Labels
A-sql-datatypes SQL column types usable in table descriptors. A-sql-pgcompat Semantic compatibility with PostgreSQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. X-anchored-telemetry The issue number is anchored by telemetry references.
Milestone

Comments

@knz
Copy link
Contributor

knz commented May 25, 2018

Following up to #25224.

Currently TIMETZ does not store the time zone.
The code could be adapted to do this and make the data type more compatible with postgres.

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-datatypes SQL column types usable in table descriptors. labels May 25, 2018
@knz knz mentioned this issue May 25, 2018
@bobvawter
Copy link
Member

Is there a similar need for TIMESTAMPTZ, too?

@jordanlewis
Copy link
Member

Luckily, no. TIMESTAMPTZ behaves like we thought TIMETZ did - no timezone stored, interpreted according to session timestamp at time of read.

@knz
Copy link
Contributor Author

knz commented Jun 5, 2018

@jordanlewis what do you think will be the timeline on this?

Unless you propose to come up with something soon-ish, I suggest we create an intermediate step where the feature is disabled. I already filed cockroachdb/docs#3254 to remove it from docs.

@jordanlewis
Copy link
Member

I'll be working on this within the next 2 weeks.

@knz knz added this to the 2.1 milestone Jul 23, 2018
@jordanlewis jordanlewis assigned knz and unassigned jordanlewis Sep 5, 2018
@jordanlewis jordanlewis modified the milestones: 2.1, Later Sep 5, 2018
@knz knz added the X-anchored-telemetry The issue number is anchored by telemetry references. label Nov 22, 2018
@knz knz removed their assignment Apr 1, 2019
jordanlewis added a commit to jordanlewis/cockroach that referenced this issue Oct 2, 2019
The spreadsheet we discussed is unwieldy - hard to edit and impossible to keep
up to date. If we write down blacklists in code, then we can use an approach
like this to always have an up to date aggregation.

So far it seems like there's just a lot of unknowns to categorize still.

The output today:

```
=== RUN   TestBlacklists
 648: unknown                                                (unknown)
 493: cockroachdb#5807   (sql: Add support for TEMP tables)
 151: cockroachdb#17511  (sql: support stored procedures)
  86: cockroachdb#26097  (sql: make TIMETZ more pg-compatible)
  56: cockroachdb#10735  (sql: support SQL savepoints)
  55: cockroachdb#32552  (multi-dim arrays)
  55: cockroachdb#26508  (sql: restricted DDL / DML inside transactions)
  52: cockroachdb#32565  (sql: support optional TIME precision)
  39: cockroachdb#243    (roadmap: Blob storage)
  33: cockroachdb#26725  (sql: support postgres' API to handle blob storage (incl lo_creat, lo_from_bytea))
  31: cockroachdb#27793  (sql: support custom/user-defined base scalar (primitive) types)
  24: cockroachdb#12123  (sql: Can't drop and replace a table within a transaction)
  24: cockroachdb#26443  (sql: support user-defined schemas between database and table)
  20: cockroachdb#21286  (sql: Add support for geometric types)
  18: cockroachdb#6583   (sql: explicit lock syntax (SELECT FOR {SHARE,UPDATE} {skip locked,nowait}))
  17: cockroachdb#22329  (Support XA distributed transactions in CockroachDB)
  16: cockroachdb#24062  (sql: 32 bit SERIAL type)
  16: cockroachdb#30352  (roadmap:when CockroachDB  will support cursor?)
  12: cockroachdb#27791  (sql: support RANGE types)
   8: cockroachdb#40195  (pgwire: multiple active result sets (portals) not supported)
   8: cockroachdb#6130   (sql: add support for key watches with notifications of changes)
   5: Expected Failure                                       (unknown)
   5: cockroachdb#23468  (sql: support sql arrays of JSONB)
   5: cockroachdb#40854  (sql: set application_name from connection string)
   4: cockroachdb#35879  (sql: `default_transaction_read_only` should also accept 'on' and 'off')
   4: cockroachdb#32610  (sql: can't insert self reference)
   4: cockroachdb#40205  (sql: add non-trivial implementations of FOR UPDATE, FOR NO KEY UPDATE, FOR SHARE, FOR NO KEY SHARE)
   4: cockroachdb#35897  (sql: unknown function: pg_terminate_backend())
   4: cockroachdb#4035   (sql/pgwire: missing support for row count limits in pgwire)
   3: cockroachdb#27796  (sql: support user-defined DOMAIN types)
   3: cockroachdb#3781   (sql: Add Data Type Formatting Functions)
   3: cockroachdb#40476  (sql: support `FOR {UPDATE,SHARE} {SKIP LOCKED,NOWAIT}`)
   3: cockroachdb#35882  (sql: support other character sets)
   2: cockroachdb#10028  (sql: Support view queries with star expansions)
   2: cockroachdb#35807  (sql: INTERVAL output doesn't match PG)
   2: cockroachdb#35902  (sql: large object support)
   2: cockroachdb#40474  (sql: support `SELECT ... FOR UPDATE OF` syntax)
   1: cockroachdb#18846  (sql: Support CIDR column type)
   1: cockroachdb#9682   (sql: implement computed indexes)
   1: cockroachdb#31632  (sql: FK options (deferrable, etc))
   1: cockroachdb#24897  (sql: CREATE OR REPLACE VIEW)
   1: pass?                                                  (unknown)
   1: cockroachdb#36215  (sql: enable setting standard_conforming_strings to off)
   1: cockroachdb#32562  (sql: support SET LOCAL and txn-scoped session variable changes)
   1: cockroachdb#36116  (sql: psychopg: investigate how `'infinity'::timestamp` is presented)
   1: cockroachdb#26732  (sql: support the binary operator: <int> / <float>)
   1: cockroachdb#23299  (sql: support coercing string literals to arrays)
   1: cockroachdb#36115  (sql: psychopg: investigate if datetimetz is being returned instead of datetime)
   1: cockroachdb#26925  (sql: make the CockroachDB integer types more compatible with postgres)
   1: cockroachdb#21085  (sql: WITH RECURSIVE (recursive common table expressions))
   1: cockroachdb#36179  (sql: implicity convert date to timestamp)
   1: cockroachdb#36118  (sql: Cannot parse '24:00' as type time)
   1: cockroachdb#31708  (sql: support current_time)
```

Release justification: non-production change
Release note: None
jordanlewis added a commit to jordanlewis/cockroach that referenced this issue Oct 24, 2019
The spreadsheet we discussed is unwieldy - hard to edit and impossible to keep
up to date. If we write down blacklists in code, then we can use an approach
like this to always have an up to date aggregation.

So far it seems like there's just a lot of unknowns to categorize still.

The output today:

```
=== RUN   TestBlacklists
 648: unknown                                                (unknown)
 493: cockroachdb#5807   (sql: Add support for TEMP tables)
 151: cockroachdb#17511  (sql: support stored procedures)
  86: cockroachdb#26097  (sql: make TIMETZ more pg-compatible)
  56: cockroachdb#10735  (sql: support SQL savepoints)
  55: cockroachdb#32552  (multi-dim arrays)
  55: cockroachdb#26508  (sql: restricted DDL / DML inside transactions)
  52: cockroachdb#32565  (sql: support optional TIME precision)
  39: cockroachdb#243    (roadmap: Blob storage)
  33: cockroachdb#26725  (sql: support postgres' API to handle blob storage (incl lo_creat, lo_from_bytea))
  31: cockroachdb#27793  (sql: support custom/user-defined base scalar (primitive) types)
  24: cockroachdb#12123  (sql: Can't drop and replace a table within a transaction)
  24: cockroachdb#26443  (sql: support user-defined schemas between database and table)
  20: cockroachdb#21286  (sql: Add support for geometric types)
  18: cockroachdb#6583   (sql: explicit lock syntax (SELECT FOR {SHARE,UPDATE} {skip locked,nowait}))
  17: cockroachdb#22329  (Support XA distributed transactions in CockroachDB)
  16: cockroachdb#24062  (sql: 32 bit SERIAL type)
  16: cockroachdb#30352  (roadmap:when CockroachDB  will support cursor?)
  12: cockroachdb#27791  (sql: support RANGE types)
   8: cockroachdb#40195  (pgwire: multiple active result sets (portals) not supported)
   8: cockroachdb#6130   (sql: add support for key watches with notifications of changes)
   5: Expected Failure                                       (unknown)
   5: cockroachdb#23468  (sql: support sql arrays of JSONB)
   5: cockroachdb#40854  (sql: set application_name from connection string)
   4: cockroachdb#35879  (sql: `default_transaction_read_only` should also accept 'on' and 'off')
   4: cockroachdb#32610  (sql: can't insert self reference)
   4: cockroachdb#40205  (sql: add non-trivial implementations of FOR UPDATE, FOR NO KEY UPDATE, FOR SHARE, FOR NO KEY SHARE)
   4: cockroachdb#35897  (sql: unknown function: pg_terminate_backend())
   4: cockroachdb#4035   (sql/pgwire: missing support for row count limits in pgwire)
   3: cockroachdb#27796  (sql: support user-defined DOMAIN types)
   3: cockroachdb#3781   (sql: Add Data Type Formatting Functions)
   3: cockroachdb#40476  (sql: support `FOR {UPDATE,SHARE} {SKIP LOCKED,NOWAIT}`)
   3: cockroachdb#35882  (sql: support other character sets)
   2: cockroachdb#10028  (sql: Support view queries with star expansions)
   2: cockroachdb#35807  (sql: INTERVAL output doesn't match PG)
   2: cockroachdb#35902  (sql: large object support)
   2: cockroachdb#40474  (sql: support `SELECT ... FOR UPDATE OF` syntax)
   1: cockroachdb#18846  (sql: Support CIDR column type)
   1: cockroachdb#9682   (sql: implement computed indexes)
   1: cockroachdb#31632  (sql: FK options (deferrable, etc))
   1: cockroachdb#24897  (sql: CREATE OR REPLACE VIEW)
   1: pass?                                                  (unknown)
   1: cockroachdb#36215  (sql: enable setting standard_conforming_strings to off)
   1: cockroachdb#32562  (sql: support SET LOCAL and txn-scoped session variable changes)
   1: cockroachdb#36116  (sql: psychopg: investigate how `'infinity'::timestamp` is presented)
   1: cockroachdb#26732  (sql: support the binary operator: <int> / <float>)
   1: cockroachdb#23299  (sql: support coercing string literals to arrays)
   1: cockroachdb#36115  (sql: psychopg: investigate if datetimetz is being returned instead of datetime)
   1: cockroachdb#26925  (sql: make the CockroachDB integer types more compatible with postgres)
   1: cockroachdb#21085  (sql: WITH RECURSIVE (recursive common table expressions))
   1: cockroachdb#36179  (sql: implicity convert date to timestamp)
   1: cockroachdb#36118  (sql: Cannot parse '24:00' as type time)
   1: cockroachdb#31708  (sql: support current_time)
```

Release justification: non-production change
Release note: None
craig bot pushed a commit that referenced this issue Nov 7, 2019
41252: roachtest: add test that aggregates orm blacklist failures r=jordanlewis a=jordanlewis

The spreadsheet we discussed is unwieldy - hard to edit and impossible to keep
up to date. If we write down blacklists in code, then we can use an approach
like this to always have an up to date aggregation.

So far it seems like there's just a lot of unknowns to categorize still.

The output today:

```
=== RUN   TestBlacklists
 648: unknown                                                (unknown)
 493: #5807   (sql: Add support for TEMP tables)
 151: #17511  (sql: support stored procedures)
  86: #26097  (sql: make TIMETZ more pg-compatible)
  56: #10735  (sql: support SQL savepoints)
  55: #32552  (multi-dim arrays)
  55: #26508  (sql: restricted DDL / DML inside transactions)
  52: #32565  (sql: support optional TIME precision)
  39: #243    (roadmap: Blob storage)
  33: #26725  (sql: support postgres' API to handle blob storage (incl lo_creat, lo_from_bytea))
  31: #27793  (sql: support custom/user-defined base scalar (primitive) types)
  24: #12123  (sql: Can't drop and replace a table within a transaction)
  24: #26443  (sql: support user-defined schemas between database and table)
  20: #21286  (sql: Add support for geometric types)
  18: #6583   (sql: explicit lock syntax (SELECT FOR {SHARE,UPDATE} {skip locked,nowait}))
  17: #22329  (Support XA distributed transactions in CockroachDB)
  16: #24062  (sql: 32 bit SERIAL type)
  16: #30352  (roadmap:when CockroachDB  will support cursor?)
  12: #27791  (sql: support RANGE types)
   8: #40195  (pgwire: multiple active result sets (portals) not supported)
   8: #6130   (sql: add support for key watches with notifications of changes)
   5: Expected Failure                                       (unknown)
   5: #23468  (sql: support sql arrays of JSONB)
   5: #40854  (sql: set application_name from connection string)
   4: #35879  (sql: `default_transaction_read_only` should also accept 'on' and 'off')
   4: #32610  (sql: can't insert self reference)
   4: #40205  (sql: add non-trivial implementations of FOR UPDATE, FOR NO KEY UPDATE, FOR SHARE, FOR NO KEY SHARE)
   4: #35897  (sql: unknown function: pg_terminate_backend())
   4: #4035   (sql/pgwire: missing support for row count limits in pgwire)
   3: #27796  (sql: support user-defined DOMAIN types)
   3: #3781   (sql: Add Data Type Formatting Functions)
   3: #40476  (sql: support `FOR {UPDATE,SHARE} {SKIP LOCKED,NOWAIT}`)
   3: #35882  (sql: support other character sets)
   2: #10028  (sql: Support view queries with star expansions)
   2: #35807  (sql: INTERVAL output doesn't match PG)
   2: #35902  (sql: large object support)
   2: #40474  (sql: support `SELECT ... FOR UPDATE OF` syntax)
   1: #18846  (sql: Support CIDR column type)
   1: #9682   (sql: implement computed indexes)
   1: #31632  (sql: FK options (deferrable, etc))
   1: #24897  (sql: CREATE OR REPLACE VIEW)
   1: pass?                                                  (unknown)
   1: #36215  (sql: enable setting standard_conforming_strings to off)
   1: #32562  (sql: support SET LOCAL and txn-scoped session variable changes)
   1: #36116  (sql: psychopg: investigate how `'infinity'::timestamp` is presented)
   1: #26732  (sql: support the binary operator: <int> / <float>)
   1: #23299  (sql: support coercing string literals to arrays)
   1: #36115  (sql: psychopg: investigate if datetimetz is being returned instead of datetime)
   1: #26925  (sql: make the CockroachDB integer types more compatible with postgres)
   1: #21085  (sql: WITH RECURSIVE (recursive common table expressions))
   1: #36179  (sql: implicity convert date to timestamp)
   1: #36118  (sql: Cannot parse '24:00' as type time)
   1: #31708  (sql: support current_time)
```

Release justification: non-production change
Release note: None

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@solongordon solongordon removed this from the Later milestone Nov 7, 2019
@otan
Copy link
Contributor

otan commented Nov 12, 2019

Done some digging and will start implementation on this shortly.

In postgres, TimeTZADT is stored as {timeofday, zone_offset}. However, zone_offset is not quite what you expect naturally - it is the negative of the offset given by t.Zone(). For example, ET is -05:00 but the offset is stored as 18000.

For comparisons, it compares timeofday, and then zone_offset. Importantly from this discovery, timetzs, even when they are equivalent in real world clock time are not UNLESS the timezones are the same. For an illustration:

 otan=# select '19:00+03'::timetz < '16:00+00'::timetz;
 ?column?
----------
 t
(1 row)

otan=# select '19:00+03'::timetz = '16:00+00'::timetz;
 ?column?
----------
 f
(1 row)

otan=# select '19:00+03'::timetz < '13:00-03'::timetz;
 ?column?
----------
 t
(1 row)

otan=# select '19:00+03'::timetz < '16:00'::time;
 ?column?
----------
 t
(1 row)

My plan is to bring back #28095 over smaller, reviewable components, but largely adapting the encoding used by #27605 because it would've been correct (minus some test failures which I will look at addressing).

btree proof: https://sourcegraph.com/github.com/postgres/postgres@7a0574b50ee9c2b96ce94c29e031c103285c0b1d/-/blob/contrib/btree_gist/btree_time.c#L181

craig bot pushed a commit that referenced this issue Nov 20, 2019
42481: sql: add types, datum and encoding for TimeTZ r=solongordon a=otan

Refs: #26097

This PR adds the types to the type system to support TimeTZ, adding the
tree.Datum class to ensure it can be parsed and then also adds the
encoding for TimeTZ to be put in the database. This is the minimum
amount of work needed before any set of tests can pass - trying to keep
the PRs small but in the end did not succeed.

Changes per package (and recommended review order) are as follows:

**sql/types: added the new TimeTZ type and type family.**

**pkg/util/timetz: introduce new TimeTZ type**

Added a custom wrapper around timeofday and offset seconds, which form
follows postgres' implementation of `TimeTzADT`. Note that `OffsetSecs`
being negative / not what you expect is fully handled by all utility
functions from this interface.

**pkg/sql/sem/tree: add Datum for DTimeTZ**

A wrapper around TimeTZ for the tree expressions.
Also removed some magic constants I found whilst form following other
things.

**pkg/util/timeutil/pgdate: remove usage of testutil.IsError**

This causes a circular dependency on the new timetz type, and this was a
very straightforward way of going around it.

**pkg/util/encoding: added encoding for DTimeTZ**
Form following postgres, we store the timeofday and offset into the
database as 12 bytes - an 8 and 4 byte varint. To have sort order right,
and to form follow the btree encoding for TimeTZ, we use UTC time when
storing it for `(En|De)codeTimeTZ[As|Des]cending`.

**pkg/sql/pgwire: added pgwire for DTimeTZ**

**pkg/sql/sqlbase: added necessary changes to encode TimeTZ data**

**ccl/changefeedccl: added encoding for ccl**
Note we must use the string type here as we cannot encode two ints
nicely with avro, nor is one int enough to not lose data.

**sql/parser: added parsing support for TimeTZ**
I wanted to omit this in this PR but some of the automated tests
iterating over every time needs this. Note there are more bits to
implement which I will leave for a later PR.

**pkg/cli: add ability to dump TimeTZ**

**sql/logictest/testdata/logic_test: added basic tests for TimeTZ**
More to come, but here are some now since the parser is all hooked up.

As this is not the fully featured commit yet, I am omitting the release
note until the test suite is more fleshed out.

Release note: none

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Nov 20, 2019
42613: sql: add cast support to TimeTZ r=otan a=otan

Refs: #26097

Added the various casts supported by postgres for `timetz`:

```
otan=# select
  castcontext,
  f.typname as "from",
  t.typname as to
from pg_cast
join pg_type as f ON (f.typelem = castsource)
join pg_type as t on (t.typelem=casttarget)
where
  castsource = 'timetz'::regtype or
  casttarget = 'timetz'::regtype;
 castcontext |     from     |   to
-------------+--------------+---------
 i           | _time        | _timetz
 a           | _timestamptz | _timetz
 i           | _timetz      | _timetz
 a           | _timetz      | _time
(4 rows)
```

No release not required as this will be combined with a fully fleshed
note for TimeTZ.

Release note: None

42622: builtins: make date_trunc(week, timestamp[tz]) truncate to Monday r=otan a=otan

Resolves #42612

Release note (sql change): The `date_trunc` function, when used with
`week`, previously truncated to Sunday. This PR will make it follow
postgres more closely by truncating to Monday instead.

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Nov 20, 2019
42618: builtins: make extract work with TimeTZ r=otan a=otan

Refs: #26097

Add extract for TimeTZ, which has a slight extension to that of Time and
a gotcha in epoch.

No release note as this will be combined in a later PR.

Release note: none

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Dec 3, 2019
42668: sql: add precision support for TIME/TIMETZ r=otan a=otan

Refs: #32565
Refs: #26097 

Adding support for precision for both TIME and TIMETZ. This also
includes threading through some extra parsing syntax for TIMETZ.

ALTER TABLE between TIME and TIMETZ not supported as they have different
representations.

Release note (sql change): This PR adds new support for precision for
TIME types (e.g. TIME(3) will truncate to milliseconds). Previously this
would raise syntax errors.

42864: rfcs: new RFC on fixing the halloween problem r=knz a=knz

I am splitting this RFC away from the [RFC on savepoints](#41569) because I know recognize it is a different problem with an orthogonal solution.

https://github.com/knz/cockroach/blob/20191129-rfc-halloween/docs/RFCS/20191014_halloween.md

There is no new idea in this text (I copy-pasted the text from the other RFC) and the implementation PRs are already out, so if the reviewers are satisfied I would gladly already merge the RFC as in-progress or completed.

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
craig bot pushed a commit that referenced this issue Dec 3, 2019
42687: sql: complete eval functionality for TimeTZ r=otan a=otan

Refs: #26097

We want to support all operations that are supported by TimeTZ in
postgres:
* equality, less than
* timetz + date, interval + timetz
* timetz + interval, interval + timetz
* timetz - interval

```
otan=# select oprname, oprcode from pg_operator
where oprleft = 'timetz'::regtype or oprright = 'timetz'::regtype
     or oprresult = 'timetz'::regtype;
 oprname |      oprcode
---------+--------------------
 =       | timetz_eq
 <>      | timetz_ne
 <       | timetz_lt
 <=      | timetz_le
 >       | timetz_gt
 >=      | timetz_ge
 +       | datetimetz_pl
 +       | timetzdate_pl
 +       | timetz_pl_interval
 -       | timetz_mi_interval
 +       | interval_pl_timetz
(11 rows)
```

Release note: None

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Dec 6, 2019
42928: builtins: add CURRENT_TIME functionality r=otan a=otan

Resolves #31708
Refs #26097

This PR adds CURRENT_TIME as a builtin.

Release note (sql change): This PR adds the CURRENT_TIME builtin, which
can be used with precision, e.g. `SELECT CURRENT_TIME, CURRENT_TIME(3)`.

43012: sql: fix current_timestamp behaviour with time zone set r=otan a=otan

With time zones set, current_timestamp needs to localise to the timezone
set in the context for time options, e.g. for `TIME` with `UTC+3` at
`UTC midnight`, `current_timestamp()` should return `3am`. This was
previously not handled correctly by Timestamp, and is rectified in this
PR.

Release note (bug fix): Previously, current_timestamp would not
correctly account for `SET TIME ZONE` in the background when storing
results, storing the timestamp as `UTC` instead. This is fixed in this
PR.

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
@craig craig bot closed this as completed in 00a58ee Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-datatypes SQL column types usable in table descriptors. A-sql-pgcompat Semantic compatibility with PostgreSQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. X-anchored-telemetry The issue number is anchored by telemetry references.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants