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: support optional TIMESTAMP precision #32098

Closed
knz opened this issue Nov 1, 2018 · 26 comments · Fixed by #42633
Closed

sql: support optional TIMESTAMP precision #32098

knz opened this issue Nov 1, 2018 · 26 comments · Fixed by #42633
Assignees
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-starter Might be suitable for a starter project for new employees or team members. X-anchored-telemetry The issue number is anchored by telemetry references.

Comments

@knz
Copy link
Contributor

knz commented Nov 1, 2018

Is your feature request related to a problem? Please describe.

CREATE TABLE t(x TIMESTAMP(6)) should work

See: https://www.postgresql.org/docs/11/static/datatype-datetime.html#DATATYPE-TIMEZONES

time, timestamp, and interval accept an optional precision value p which specifies the number of fractional digits retained in the seconds field. By default, there is no explicit bound on precision. The allowed range of p is from 0 to 6.

Describe the solution you'd like

MVP: An optional precision of 6 should be recognized and ignored (this is the default in CockroachDB)

Optionally: support for other precisions than 6.

Workaround

If the desired precision is 6, then the precision can be omitted altogether to obtain equivalent behavior.

@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-pgcompat Semantic compatibility with PostgreSQL labels Nov 1, 2018
@knz knz added this to Triage in (DEPRECATED) SQL Front-end, Lang & Semantics via automation Nov 1, 2018
@knz
Copy link
Contributor Author

knz commented Nov 1, 2018

Note: when we support this feature we can re-introduce nanosecond precision (eg via timestamp(9)) and simply keep 6 as default for pg compat.

@knz
Copy link
Contributor Author

knz commented Nov 1, 2018

cc @mjibson we discussed that today

@maddyblue
Copy link
Contributor

maddyblue commented Nov 3, 2018

[Moved to #32143]

@knz
Copy link
Contributor Author

knz commented Nov 3, 2018 via email

@maddyblue
Copy link
Contributor

Oops. Moved to #32143.

@knz knz moved this from Triage to Backlog in (DEPRECATED) SQL Front-end, Lang & Semantics Nov 21, 2018
@knz knz changed the title sql: support optional timestamp precision sql: support optional TIMESTAMP precision Nov 22, 2018
@knz knz added the X-anchored-telemetry The issue number is anchored by telemetry references. label Nov 22, 2018
knz added a commit to knz/cockroach that referenced this issue Nov 22, 2018
…tamp with precision

This also somewhat simplifies the grammar.

Release note: None
knz added a commit to knz/cockroach that referenced this issue Nov 26, 2018
…tamp with precision

This also somewhat simplifies the grammar.

Release note: None
knz added a commit to knz/cockroach that referenced this issue Nov 28, 2018
…tamp with precision

This also somewhat simplifies the grammar.

Release note: None
@knz
Copy link
Contributor Author

knz commented Dec 19, 2018

Double check #16349 when working on this. Verify whether current_timestamp also needs to accept an optional precision.

@dalvlex
Copy link

dalvlex commented Feb 11, 2019

I ran into this issue on Symfony4/Doctrine, CREATE TABLE fails because of TIMESTAMP precision.

Symfony4 log

In AbstractPostgreSQLDriver.php line 79:
                                                                                                                                                                                      
  An exception occurred while executing 'CREATE TABLE migration_versions (version VARCHAR(14) NOT NULL, executed_at TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, PRIMARY KEY(version))':  
                                                                                                                                                                                      
  SQLSTATE[0A000]: Feature not supported: 7 ERROR:  unimplemented at or near "zone"                                                                                                   
  DETAIL:  source SQL:                                                                                                                                                                
  CREATE TABLE migration_versions (version VARCHAR(14) NOT NULL, executed_at TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, PRIMARY KEY(version))                                           
                                                                                                       ^                                                                              
  HINT:  See: https://github.com/cockroachdb/cockroach/issues/32098                                                                                                                                                                                        

I installed the latest CockroachDB v2.1.4 (and later v2.2.0-alpha.20190114 also), set up a Symfony4/Doctrine project, configured it with pdo_pgsql for CockroachDB, created a database with php bin/console doctrine:database:create, then created an entity as described on https://symfony.com/doc/current/doctrine.html, and ran the migration with php bin/console make:migration at which point it gave the above errors.

If the SQL is changed to CREATE TABLE migration_versions (version VARCHAR(14) NOT NULL, executed_at TIMESTAMP WITHOUT TIME ZONE NOT NULL, PRIMARY KEY(version));, without specifying TIMESTAMP precision, it works.

This is related to #16349 too because Symfony4/Doctrine specifically ask for TIMESTAMP(0), but at the moment it is not accepting any kind of precision value.

In Symfony4/Doctrine this issue can be worked around by deleting all the precision entries from the vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php, after which the migration creates the database tables and subsequently throws the following error:

In AbstractPostgreSQLDriver.php line 79:
                                                                                                                                                                       
  An exception occurred while executing 'SELECT                                                                                                                        
                      a.attnum,                                                                                                                                        
                      quote_ident(a.attname) AS field,                                                                                                                 
                      t.typname AS type,                                                                                                                               
                      format_type(a.atttypid, a.atttypmod) AS complete_type,                                                                                           
                      (SELECT t1.typname FROM pg_catalog.pg_type t1 WHERE t1.oid = t.typbasetype) AS domain_type,                                                      
                      (SELECT format_type(t2.typbasetype, t2.typtypmod) FROM                                                                                           
                        pg_catalog.pg_type t2 WHERE t2.typtype = 'd' AND t2.oid = a.atttypid) AS domain_complete_type,                                                 
                      a.attnotnull AS isnotnull,                                                                                                                       
                      (SELECT 't'                                                                                                                                      
                       FROM pg_index                                                                                                                                   
                       WHERE c.oid = pg_index.indrelid                                                                                                                 
                          AND pg_index.indkey[0] = a.attnum                                                                                                            
                          AND pg_index.indisprimary = 't'                                                                                                              
                      ) AS pri,                                                                                                                                        
                      (SELECT pg_get_expr(adbin, adrelid)                                                                                                              
                       FROM pg_attrdef                                                                                                                                 
                       WHERE c.oid = pg_attrdef.adrelid                                                                                                                
                          AND pg_attrdef.adnum=a.attnum                                                                                                                
                      ) AS default,                                                                                                                                    
                      (SELECT pg_description.description                                                                                                               
                          FROM pg_description WHERE pg_description.objoid = c.oid AND a.attnum = pg_description.objsubid                                               
                      ) AS comment                                                                                                                                     
                      FROM pg_attribute a, pg_class c, pg_type t, pg_namespace n                                                                                       
                      WHERE n.nspname NOT IN ('pg_catalog', 'information_schema', 'pg_toast') AND c.relname = 'backward_dependencies' AND n.nspname = 'crdb_internal'  
                          AND a.attnum > 0                                                                                                                             
                          AND a.attrelid = c.oid                                                                                                                       
                          AND a.atttypid = t.oid                                                                                                                       
                          AND n.oid = c.relnamespace                                                                                                                   
                      ORDER BY a.attnum':                                                                                                                              
                                                                                                                                                                       
  SQLSTATE[XX000]: Internal error: 7 ERROR:  could not decorrelate subquery                                                                                            

This one leaves me utterly clueless, but I saw this specific query was mentioned by @Freeaqingme on #3288.

@knz
Copy link
Contributor Author

knz commented Feb 11, 2019

Thanks for your report!

I extracted the request about the correlated subquery into a separate issue #34776

craig bot pushed a commit that referenced this issue Feb 21, 2019
35128: sql: accept+ignore precision 6 on TIME,TIMESTAMP,TIMESTAMPTZ r=knz a=knz

Informs #32098.
Informs #32565.
This is not the full feature but might unlock Flowable compat. cc @rolandcrosby @awoods187  @drewdeally 

Release note (sql change): CockroachDB now recognizes the syntax
`TIME(6)`, `TIMESTAMP(6)` and `TIMESTAMPTZ(6)` / `TIMESTAMP(6) WITH
TIME ZONE`, for compatibility with PostgreSQL. Only the value 6 is
supported (which is also the default in PostgreSQL). When used for a
table column definition, the precision is not stored and it is not
possible to distinguish types with and without specified precisions in
the introspection metadata.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@awoods187
Copy link
Contributor

@knz was this addressed with your change?

@knz
Copy link
Contributor Author

knz commented Mar 21, 2019 via email

@solongordon solongordon assigned otan and unassigned rohany Nov 13, 2019
craig bot pushed a commit that referenced this issue Nov 21, 2019
41788: storage/rangefeed: push rangefeed.Filter into Raft processing goroutine r=danhhz a=nvanbenschoten

This commit creates a `rangefeed.Filter` object, which informs the producer of logical operations of the information that a `rangefeed.Processor` is interested in, given its current set of registrations. It can be used to avoid performing extra work to provide the Processor with information which will be ignored.

This is an important optimization for single-key or small key span rangefeeds, as it avoids some extraneous work for the key spans not being watched.

For now, this extra work is just fetching the post-operation value for `MVCCWriteValueOp` and `MVCCCommitIntentOp` operations, but this can be extended in the future to include the pre-operation value as well. This will be important to avoid any perf regression when addressing #28666.

42580: sql: add precision support for TIMESTAMP/TIMESTAMPTZ (v2) r=otan a=otan

Refs #32098
Alternative to #42552

----

Add precision support for TIMESTAMP/TIMESTAMPTZ, supporting precisions
from 0 to 6 inclusive.

When storing this in the type proto, we introduce a `TimePrecisionIsSet`
variable, which is used to differentiate 0 (unset) or 0 (explicitly set)
for time related protos. This is abstracted away in the `Precision()`
function which returns the correct precision (unset => 6) for time. This
allows forwards and backwards compatibility.

Note Time will come later, as it involves a bit more plumbing to the
base library.

Release note (sql change): Introduces precision support for TIMESTAMP
and TIMESTAMPTZ, supporting precisions from 0 to 6 inclusive. Previous
versions of TIMESTAMP and TIMESTAMPTZ will default to 6 units of
precision. Note that if anyone downgrades whilst having precision set,
they will have full precision (6) again, but if they re-upgrade they
will find their precisions truncated again.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
@craig craig bot closed this as completed in 6cc49fa Nov 21, 2019
SQL Features (Deprecated - use SQL Experience board) automation moved this from To do to Done Nov 21, 2019
@bryanhuntesl
Copy link

This fix has not made it into cockroachdb/cockroach:v19.1.6 / cockroachdb/cockroach:v2.1.10 or cockroachdb/cockroach:latest - all of which are producing the same error message :

** (Postgrex.Error) ERROR 0A000 (feature_not_supported) syntax error: unimplemented: unimplemented at or near ","

    query: CREATE TABLE IF NOT EXISTS "schema_migrations" ("version" bigint, "inserted_at" timestamp(0), PRIMARY KEY ("version"))

    hint: See: https://github.com/cockroachdb/cockroach/issues/32098

source SQL:
CREATE TABLE IF NOT EXISTS "schema_migrations" ("version" bigint, "inserted_at" timestamp(0), PRIMARY KEY ("version"))
                                                                                            ^
    (ecto_sql) lib/ecto/adapters/sql.ex:629: Ecto.Adapters.SQL.raise_sql_call_error/1
    (elixir) lib/enum.ex:1336: Enum."-map/2-lists^map/1-0-"/2
    (ecto_sql) lib/ecto/adapters/sql.ex:716: Ecto.Adapters.SQL.execute_ddl/4
    (ecto_sql) lib/ecto/migrator.ex:633: Ecto.Migrator.verbose_schema_migration/3
    (ecto_sql) lib/ecto/migrator.ex:477: Ecto.Migrator.lock_for_migrations/4
    (ecto_sql) lib/ecto/migrator.ex:401: Ecto.Migrator.run/4
    (ecto_sql) lib/ecto/migrator.ex:142: Ecto.Migrator.with_repo/3
    (ecto_sql) lib/mix/tasks/ecto.migrate.ex:118: anonymous fn/5 in Mix.Tasks.Ecto.Migrate.run/2
    (elixir) lib/enum.ex:1948: Enum."-reduce/3-lists^foldl/2-0-"/3
    (ecto_sql) lib/mix/tasks/ecto.migrate.ex:106: Mix.Tasks.Ecto.Migrate.run/2
    (mix) lib/mix/task.ex:331: Mix.Task.run_task/3
    (mix) lib/mix/task.ex:365: Mix.Task.run_alias/3
    (mix) lib/mix/task.ex:292: Mix.Task.run/2
    (mix) lib/mix/cli.ex:79: Mix.CLI.run_task/2
    (elixir) lib/code.ex:813: Code.require_file/2

@rohany
Copy link
Contributor

rohany commented Jan 8, 2020

It does not look like #42633 has been backported to those releases. @otan is it possible to do this?

@knz
Copy link
Contributor Author

knz commented Jan 8, 2020

As a matter of process, we don't backport features to previous releases - only bug fixes and (for 2 releases back) security fixes.

This lack of support is not a bug.

@bryanhuntesl
Copy link

Is there a released (docker) version with this fix included ?

@bryanhuntesl
Copy link

I was told it would be included in 20.1 alpha back in december https://cockroachdb.slack.com/archives/CP4D9LD5F/p1575642161341500?thread_ts=1575633661.339600&cid=CP4D9LD5F

@awoods187
Copy link
Contributor

Here is the 20.1 alpha with the docker information https://www.cockroachlabs.com/docs/releases/v20.1.0-alpha20191216.html

@bryanhuntesl
Copy link

Much appreciated @awoods187

@bryanhuntesl
Copy link

@awoods187 - works like a charm with cockroachdb/cockroach-unstable:v20.1.0-alpha20191216

@bryanhuntesl
Copy link

Just for the record - this fix is in cockroachdb/cockroach:v19.2.1

@otan
Copy link
Contributor

otan commented Jan 22, 2020

Just for the record - this fix is in cockroachdb/cockroach:v19.2.1

sorry to break the spirit but that...should not be the case :o
what commands are you running? afaict, we should only support timestamp(6) in there.

@DavidOliver
Copy link

DavidOliver commented Mar 4, 2020

Because support for timestamp(0) was pulled, Craft CMS, a popular PHP CMS built with the Yii framework doesn't work with CockroachDB. I believe this also re-blocks use with Elixir's Ecto.

Could/should support be re-added in a way which avoids the issues that PR addresses? If so, should this issue be re-opened? Thanks!

@awoods187
Copy link
Contributor

We actually fixed this for 20.1, our upcoming release. You can see it in this pr #42580 or in a beta here https://www.cockroachlabs.com/docs/releases/v20.1.0-beta.2.html

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-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-starter Might be suitable for a starter project for new employees or team members. X-anchored-telemetry The issue number is anchored by telemetry references.
Projects
No open projects
10 participants