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: INTERVAL output doesn't match PG #35807

Closed
awoods187 opened this issue Mar 15, 2019 · 12 comments
Closed

sql: INTERVAL output doesn't match PG #35807

awoods187 opened this issue Mar 15, 2019 · 12 comments
Labels
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. meta-issue Contains a list of several other issues. S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.

Comments

@awoods187
Copy link
Contributor

awoods187 commented Mar 15, 2019

In pg (http://www.postgresqltutorial.com/postgresql-interval/):

SELECT
 now(),
 now() - INTERVAL '1 year 3 hours 20 minutes' 
             AS "3 hours 20 minutes ago of last year";

image

In master (v19.1.0-beta.20190304-458-g70e3468),

root@localhost:26257/tpcc> SELECT                                                                                                                                                                                                                                         now(),                                                                                                                                                                                                                                                                   now() - INTERVAL '1 year 3 hours 20 minutes'                                                                                                                                                                                                                                         AS "3 hours 20 minutes ago of last year";
                now                | 3 hours 20 minutes ago of last year
+----------------------------------+-------------------------------------+
  2019-03-15 21:53:18.633297+00:00 | 2018-03-15 18:33:18.633297+00:00
(1 row)

Time: 15.117597ms

knz edit: this should be rejected an unimplemented error. The fact we accept this silently and do the wrong thing is a bug. Please file separately. Thanks. The reference example you used is outdated. Run this in a new postgres to see the behavior is the same.

Update
Filed #35874

@awoods187
Copy link
Contributor Author

awoods187 commented Mar 15, 2019

In pg:

SELECT
    EXTRACT (
        MINUTE
        FROM
            INTERVAL '5 hours 21 minutes'
    );
21

Returns
In crdb:

root@localhost:26257/tpcc> SELECT                                                                                                                                                                                                                                            EXTRACT (                                                                                                                                                                                                                                                                    MINUTE                                                                                                                                                                                                                                                                   FROM                                                                                                                                                                                                                                                                         INTERVAL '5 hours 21 minutes'                                                                                                                                                                                                                                    );
pq: unknown signature: extract(string, interval)

knz edit: I had started a PR for this #27502 but it was never complete. Don't remember why. We also don't have telemetry for this.
Update
This is non-urgent but we should probably finish that PR/add telemetry.

@awoods187
Copy link
Contributor Author

awoods187 commented Mar 15, 2019

None of the interval styles work in CRDB:

root@localhost:26257/tpcc> SET intervalstyle = 'sql_standard';
pq: invalid value for parameter "IntervalStyle": "sql_standard"
DETAIL: this parameter is currently recognized only for compatibility and has no effect in CockroachDB.
HINT: Available values: postgres
root@localhost:26257/tpcc> SET intervalstyle = 'postgres';
SET

Time: 6.701231ms
root@localhost:26257/tpcc> SET intervalstyle = 'postgres_verbose';
pq: invalid value for parameter "IntervalStyle": "postgres_verbose"
DETAIL: this parameter is currently recognized only for compatibility and has no effect in CockroachDB.
HINT: Available values: postgres
root@localhost:26257/tpcc> SET intervalstyle = 'iso_8601';
pq: invalid value for parameter "IntervalStyle": "iso_8601"
DETAIL: this parameter is currently recognized only for compatibility and has no effect in CockroachDB.
HINT: Available values: postgres

knz edit these are simply unimplemented features. Never was on any roadmap. We don't have telemetry for them. Should we add telemetry?
Update
Let's add telemetry but it's non-urgent

@awoods187
Copy link
Contributor Author

awoods187 commented Mar 15, 2019

Interval can't go as far back on CRDB as it can on PG:

INSERT
INTO
    intervals
VALUES
(17, INTERVAL'-178000000y');
pq: overflow during Encode
INSERT
INTO
    intervals
VALUES
(17, INTERVAL'-17800000y');
pq: overflow during Encode
INSERT
INTO
    intervals
VALUES
(17, INTERVAL'-1780000y');
pq: overflow during Encode
INSERT
INTO
    intervals
VALUES
(17, INTERVAL'-178000y');
pq: overflow during Encode
INSERT
INTO
    intervals
VALUES
(17, INTERVAL'-17800y');                                                                                                                                                                                                                                                                       
pq: overflow during Encode
INSERT
INTO
    intervals
VALUES
(17, INTERVAL'-1780y');
INSERT 1
Time: 840.276319ms
root@localhost:26257/tpcc> select * from intervals;
  a  |                 b
+----+-----------------------------------+
  17 | -1780 years

The same is true for positive values.

knz edit: BC dates are already tracked in #28099. The large values in magnitude are "just" a design difference which we should document as known limitation. Maybe we should also track in telemetry, WDYT?
Update
I definitely think we should track this as it is different than PG and perhaps may cause a problem. @rmloveland we should not this somewhere as well too

@awoods187
Copy link
Contributor Author

awoods187 commented Mar 15, 2019

Nanoseconds work in CRDB but not in PG (https://www.postgresql.org/docs/current/datatype-datetime.html)

SELECT INTERVAL '6 years 5 months 4 days 3 hours 2 minutes 1 second 1 microsecond 1 nanosecond';
                ?column?
+---------------------------------------+
  6 years 5 mons 4 days 03:02:01.000001
(1 row)

Time: 2.014446ms

knz edit: pg does not support (and rejects) the 'nanoseconds' specifier. Now that crdb does not support nanoseconds any more, the specifier should also be rejected during parsing. Please file a separate issue.
Update
Filed #35872 for this comment

@awoods187 awoods187 added 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. S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. labels Mar 15, 2019
@knz knz added the meta-issue Contains a list of several other issues. label May 20, 2019
@knz
Copy link
Contributor

knz commented May 20, 2019

@jordanlewis I think most of the issues here have been addressed, it may be possible to close this issue.

@jordanlewis
Copy link
Member

@mjibson could you please check out whether this issue is closeable? I suspect not, given what you've learned recently about intervals. If it's not closeable, could you please write down a short summary in this issue about your learnings? Thanks!

@maddyblue
Copy link
Contributor

We still don't have:

  • intervalstyle
  • extract(minute from interval)
  • the same range of possible values

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>
@aashipov
Copy link

SELECT (DATE_RANGE.END - DATE_RANGE.BEGIN) AS DATE_DIFFERENCE, EXTRACT(DAYS FROM (DATE_RANGE.END - DATE_RANGE.BEGIN)) AS DAYS
FROM
(SELECT date_trunc('microsecond', timestamp '2020-06-22 07:50:18.567579') AS END,
date_trunc('microsecond', timestamp '2019-09-01 07:50:18.552490') AS BEGIN)
AS DATE_RANGE;

Postgresql: '0 years 0 mons 295 days 0 hours 0 mins 0.015089 secs', 295
CockroachDB: '0 years 0 mons 0 days 7080 hours 0 mins 0.015089 secs', 0

@knz
Copy link
Contributor

knz commented Jun 22, 2020

cc @otan you might find this interesting

@rafiss
Copy link
Collaborator

rafiss commented Feb 6, 2021

relates a bit to #59720 which is about parsing intervals.

@otan
Copy link
Contributor

otan commented Feb 7, 2021

we're only missing intervalstyle now i think.

@rafiss
Copy link
Collaborator

rafiss commented Feb 26, 2021

Closing this meta-issue in favor of a specific one for IntervalStyle: #61167

@rafiss rafiss closed this as completed Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. meta-issue Contains a list of several other issues. S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.
Projects
None yet
Development

No branches or pull requests

7 participants