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: invalid value for parameter "TimeZone": "GMT-08:00" #41776

Closed
rafiss opened this issue Oct 21, 2019 · 5 comments · Fixed by #42781
Closed

sql: invalid value for parameter "TimeZone": "GMT-08:00" #41776

rafiss opened this issue Oct 21, 2019 · 5 comments · Fixed by #42781
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) good first issue

Comments

@rafiss
Copy link
Collaborator

rafiss commented Oct 21, 2019

The TimeZone parameter should support GMT-08:00 as a value, but it doesn't. This is used in a PGJDBC test. See https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/test/java/org/postgresql/test/jdbc2/TimezoneTest.java

@rafiss rafiss 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 Oct 21, 2019
@jordanlewis
Copy link
Member

Concrete repro of the issue (this is the same thing as what you're talking about with "parameter", right?):

[email protected]:57107/defaultdb> set timezone='GMT-08:00';
pq: invalid value for parameter "timezone": "'GMT-08:00'"
DETAIL: cannot find time zone "GMT-08:00": unknown time zone GMT-08:00

@droidnoob
Copy link
Contributor

@jordanlewis I would like to pick this up. I'm new to this. Can you guide me through?

@rohany
Copy link
Contributor

rohany commented Oct 29, 2019

It looks like this error is thrown in pkg/sql/set_var.go:226. You would probably need to update the timeutil.LoadLocation function to fix this bug.

Let me know if you have any more questions!

@rafiss
Copy link
Collaborator Author

rafiss commented Oct 29, 2019

The fix for this particular issue could be pretty simple, but see #36864 for some additional context on a larger change we may want to do later.

@droidnoob
Copy link
Contributor

So my guess is we need to take the input GMT-08:00 and instead of setting timezone with that, we use set timezone=-8. @rafiss @rohany Am I on the right track?

droidnoob added a commit to droidnoob/cockroach that referenced this issue Nov 1, 2019
…roachdb#41776

sql: added an extra string to time zone converter to check strings like GMT-08:00
there was no check in place to convert the strings to timezone. Added a regex to check for valid string and convert it to offset in timeutil package

Fix for cockroachdb#41776

Release Notes:

* SQL - Now Accepts timezone="GMT-08:00", UST+11:00, -03:00

Release note (<category, see below>): <what> <show> <why>
droidnoob added a commit to droidnoob/cockroach that referenced this issue Nov 24, 2019
…roachdb#41776

added an extra checker to time zone converter to check strings like GMT-08:00
there was no check in place to convert the strings to timezone
added a regex to check for valid string and convert it to offset in timeutil package

Fix for cockroachdb#41776

Release Notes:

* SQL - Now Accepts timezone="GMT-08:00", UST+11:00, -03:00.

Release note (<category, see below>): <what> <show> <why>
droidnoob added a commit to droidnoob/cockroach that referenced this issue Nov 26, 2019
…roachdb#41776

Previously, the inputs like `GMT+5` would cause error.
Postgres supports inputs like this. So adding support for
these inputs was necessary.
sql: Added a check to convert the strings to offset.
timeutil: added a function to convert input to offset seconds.
Added unit tests. Added a regex to accept the inputs.

Release note (sql change): `SET TIME ZONE` now accepts inputs
mentioned in cockroachdb#41776. Now accepted time zone strings
* `GMT+5`
* `UTC-03:59:59`
otan pushed a commit to otan-cockroach/cockroach that referenced this issue Nov 26, 2019
…roachdb#41776

Previously, the inputs like `GMT+5` would cause error.
Postgres supports inputs like this. So adding support for
these inputs was necessary.

Release note (sql change): `SET TIME ZONE` now accepts inputs
mentioned beginning with 'GMT' and 'UTC', such as 'GMT+5' and
'UTC-3:59'. This was previously unsupported.
droidnoob added a commit to droidnoob/cockroach that referenced this issue Nov 27, 2019
…roachdb#41776

Previously, the inputs starting with `UTC` and `GMT` would cause error.
Postgres supports inputs like this. So adding support for
these inputs was necessary.
sql: Added a check to convert the strings to offset.
timeutil: added a function to convert input to offset seconds.
Added unit tests. Added a regex to accept the above inputs.
Cleaned the code.

Release note (sql change): `SET TIME ZONE` now accepts inputs
beginning with `GMT` and `UTC`, such as `GMT+5` and
`UTC-3:59`. This was previously unsupported.
droidnoob added a commit to droidnoob/cockroach that referenced this issue Nov 27, 2019
…roachdb#41776

Previously, the inputs starting with `UTC` and `GMT` would cause error.
Postgres supports inputs like this. So adding support for
these inputs was necessary.
sql: Added a check to convert the strings to offset.
timeutil: added a function to convert input to offset seconds.
Added unit tests. Added a regex to accept the above inputs.
Cleaned the code.

Release note (sql change): `SET TIME ZONE` now accepts inputs
beginning with `GMT` and `UTC`, such as `GMT+5` and
`UTC-3:59`. This was previously unsupported.
@craig craig bot closed this as completed in f3d278e Nov 27, 2019
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) good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants