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.

Sign up for GitHub

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

Jump to bottom

sql: support special date/timestamp constants of postgresql #31954

Closed
adarshaj opened this issue Oct 28, 2018 · 1 comment · Fixed by #31758
Closed

sql: support special date/timestamp constants of postgresql #31954

adarshaj opened this issue Oct 28, 2018 · 1 comment · Fixed by #31758
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@adarshaj
Copy link

Describe the problem
I am using hikaricp and stumbled on this issue and on digging further figured that the special constants supported by postgres is not implemented in cockroachdb, causing error with keeping the connection state active in hikaricp.

Please describe the issue you observed, and any steps we can take to reproduce it:

This is the stacktrace which keeps appearing in the application log:

 WARN  c.zaxxer.hikari.pool.ProxyConnection  - db.config - Connection org.postgresql.jdbc.PgConnection@5c6f88e5 marked as broken because of SQLSTATE(08P01), ErrorCode(0)
org.postgresql.util.PSQLException: ERROR: error in argument for $1: could not parse string "infinity" as timestamp
	at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2433)
	at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2178)
	at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:306)
	at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:441)
	at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:365)
	at org.postgresql.jdbc.PgPreparedStatement.executeWithFlags(PgPreparedStatement.java:155)
	at org.postgresql.jdbc.PgPreparedStatement.execute(PgPreparedStatement.java:144)
	at com.zaxxer.hikari.pool.ProxyPreparedStatement.execute(ProxyPreparedStatement.java:44)
	at com.zaxxer.hikari.pool.HikariProxyPreparedStatement.execute(HikariProxyPreparedStatement.java)
	at slick.jdbc.StatementInvoker.results(StatementInvoker.scala:38)
	at slick.jdbc.StatementInvoker.iteratorTo(StatementInvoker.scala:21)
	at slick.jdbc.Invoker.foreach(Invoker.scala:47)
	at slick.jdbc.Invoker.foreach$(Invoker.scala:46)
	at slick.jdbc.StatementInvoker.foreach(StatementInvoker.scala:15)
	at slick.jdbc.StreamingInvokerAction.run(StreamingInvokerAction.scala:22)
	at slick.jdbc.StreamingInvokerAction.run$(StreamingInvokerAction.scala:20)
	at slick.jdbc.JdbcActionComponent$QueryActionExtensionMethodsImpl$$anon$1.run(JdbcActionComponent.scala:216)
	at slick.jdbc.JdbcActionComponent$QueryActionExtensionMethodsImpl$$anon$1.run(JdbcActionComponent.scala:216)
	at slick.basic.BasicBackend$DatabaseDef$$anon$2.liftedTree1$1(BasicBackend.scala:275)
	at slick.basic.BasicBackend$DatabaseDef$$anon$2.run(BasicBackend.scala:275)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)

To Reproduce

What did you do? Describe in your own words.
If possible, provide steps to reproduce the behavior:
I have distilled the problem to these simple sql statements -

  1. start crdb's sql client
  2. execute select 'infinity':::TIMESTAMP; and note the error you are greeted with:
pq: could not parse "infinity" as type timestamp

Expected behavior

The same statement on psql console with postgres database gives no error:

default=# select TIMESTAMP 'infinity' as "TIMESTAMP";
 TIMESTAMP 
-----------
 infinity
(1 row)

Environment:

  • CockroachDB version: 2.0.6
  • Server OS: Linux/Docker
  • Client app: cockroach sql & 'hikaricp' + slick + jdbc

Additional context
What was the impact?
This issue directly impacts on scalability of connection pooling with hikaricp. Fixing this would be make connection pooling smoother.
@petermattis petermattis added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Oct 28, 2018
@knz
Copy link
Contributor

knz commented Oct 29, 2018

Fixed by #31758.

@knz knz added the A-sql-pgcompat Semantic compatibility with PostgreSQL label Oct 29, 2018
@bobvawter bobvawter mentioned this issue Oct 29, 2018
Merged
craig bot pushed a commit that referenced this issue Oct 30, 2018
@bobvawter
Merge #31758
c8e87ae 
31758: sql: Generalize date/time parsing r=bobvawter a=bobvawter

sql: Generalize date/time parsing

The current date/time parsing code relies on `time.ParseInLocation()`. It does
not support all of the various date/time formats accepted by PostgreSQL and
also requires multiple invocation to try the various date/time formats that we
do accept.

This change updates the date/time parsing code with a new implementation that
does not delegate to `time.ParseInLocation()` and is able to parse all
supported formats in a single pass.

In order to support parsing named timezones like `America/New_York`, we
delegate to `time.LoadLocation()` as we did previously.  `LoadLocation()` is
rather expensive, since it looks for tzinfo files on disk every time it is
invoked. A per-node, in-memory cache has been added to amortize this overhead.
Per #31978, the tzinfo used on each node could already be inconsistent,
depending on the tzinfo files present in the underlying OS.

The following table compares the new `ParseTimestamp()` function to calling
`ParseInLocation()`.  While it is true that `ParseInLocation()` is generally
faster for any given pattern, the current parsing code must call it repeatedly,
trying each supported date format until one succeeds. The test with the named
timezone also shows the significant overhead of calling `LoadLocation()`.

```
2003-06-12/ParseTimestamp-8             10000000               122 ns/op          81.53 MB/s
2003-06-12/ParseInLocation-8            30000000                35.6 ns/op       281.29 MB/s
2003-06-12_01:02:03/ParseTimestamp-8            10000000               163 ns/op         116.45 MB/s
2003-06-12_01:02:03/ParseInLocation-8           30000000                54.4 ns/op       349.16 MB/s
2003-06-12_04:05:06.789-04:00/ParseTimestamp-8          10000000               238 ns/op         121.69 MB/s
2003-06-12_04:05:06.789-04:00/ParseInLocation-8         10000000               161 ns/op         180.05 MB/s
2000-01-01T02:02:02.567+09:30/ParseTimestamp-8           5000000               233 ns/op         124.01 MB/s
2000-01-01T02:02:02.567+09:30/ParseInLocation-8         10000000               158 ns/op         182.41 MB/s
2003-06-12_04:05:06.789_America/New_York/ParseTimestamp-8                3000000               475 ns/op          84.06 MB/s
2003-06-12_04:05:06.789_America/New_York/ParseInLocation-8                200000              7313 ns/op           3.15 MB/s
```

The tests in `parsing_test.go` have an optional mode to cross-check the test
data aginst a PostgreSQL server.  This is useful for developing, but is not
part of the automated build.

Parsing of BC dates is supported, #28099 could then be completed by changing
the date-formatting code to print a BC date.

This change would allow #30697 (incomplete handling of datestyle) to be
re-evaluated, since the parser does allow configuration of YMD, DMY, or MDY
input styles.

Resolves #27500
Resolves #27501
Resolves #31954

Release note (sql change): A wider variety of date, time, and timestamp formats
are now accepted by the SQL frontend.

Release note (bug fix): Prepared statements that bind temporal values now
respect the session's timezone setting. Previously, bound temporal values were
always interpreted as though the session time zone were UTC.

Release note (backward-incompatible change): Timezone abbreviations, such as
EST, are no longer allowed when parsing or converting to a date/time type.
Previously, an abbreviation would be accepted if it were an alias for the
session's timezone.


Co-authored-by: Bob Vawter <bob@cockroachlabs.com>
@craig craig bot closed this as completed in #31758 Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: Generalize date/time parsing
3 participants
@adarshaj @knz @petermattis