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

Named timezones are case sensitive #8087

Open
morgo opened this issue Oct 28, 2018 · 9 comments
Open

Named timezones are case sensitive #8087

morgo opened this issue Oct 28, 2018 · 9 comments
Labels
Priority/P4 Minor issue, awaiting more evidence before prioritizing type/compatibility

Comments

@morgo
Copy link
Contributor

morgo commented Oct 28, 2018

Bug Report

Please answer these questions before submitting your issue. Thanks!

  1. What did you do?
SET time_zone='utc';
SELECT @@time_zone;
SET time_zone='UTC';
SELECT @@time_zone;
  1. What did you expect to see?
mysql57> SET time_zone='utc';
Query OK, 0 rows affected (0.00 sec)

mysql57> SELECT @@time_zone;
+-------------+
| @@time_zone |
+-------------+
| UTC         |
+-------------+
1 row in set (0.00 sec)

mysql57> SET time_zone='UTC';
Query OK, 0 rows affected (0.00 sec)

mysql57> SELECT @@time_zone;
+-------------+
| @@time_zone |
+-------------+
| UTC         |
+-------------+
1 row in set (0.00 sec)
  1. What did you see instead?
tidb> SET time_zone='utc';
ERROR 1298 (HY000): Unknown or incorrect time zone: 'utc'
tidb> SELECT @@time_zone;
+-------------+
| @@time_zone |
+-------------+
| SYSTEM      |
+-------------+
1 row in set (0.00 sec)

tidb> SET time_zone='UTC';
Query OK, 0 rows affected (0.00 sec)

tidb> SELECT @@time_zone;
+-------------+
| @@time_zone |
+-------------+
| UTC         |
+-------------+
1 row in set (0.00 sec)
  1. What version of TiDB are you using (tidb-server -V or run select tidb_version(); on TiDB)?
tidb> SELECT tidb_version()\G
*************************** 1. row ***************************
tidb_version(): Release Version: v2.1.0-rc.3-98-gd65ce86
Git Commit Hash: d65ce8655cd26bb5ec44f9239562013af692d2fa
Git Branch: master
UTC Build Time: 2018-10-24 08:22:37
GoVersion: go version go1.11 linux/amd64
Race Enabled: false
TiKV Min Version: 2.1.0-alpha.1-ff3dd160846b7d1aed9079c389fc188f7f5ea13e
Check Table Before Drop: false
1 row in set (0.00 sec)
@morgo
Copy link
Contributor Author

morgo commented Oct 28, 2018

I can see it being a common mistake that applications specify the wrong case.

@morgo
Copy link
Contributor Author

morgo commented Apr 1, 2019

This looks like it will be hard to fix. TiDB depends on Go's time package, which is case sensitive. Earlier bugs suggesting insensitivity have been rejected: golang/go#21512

@ghost ghost added this to Issue Backlog: Need Triage in SIG Runtime Kanban via automation Aug 5, 2020
@ghost ghost added the Priority/P4 Minor issue, awaiting more evidence before prioritizing label Aug 12, 2020
@ghost ghost moved this from Issue Backlog: Need Triage to Backlog: Low Priority in SIG Runtime Kanban Aug 12, 2020
@dveeden
Copy link
Contributor

dveeden commented Oct 26, 2021

We could try to fix some common mistakes: https://play.golang.org/p/h0AdsIQ3uf_s

In the above example utc get fixed to UTC and Europe/prague gets fixed to Europe/Prague. However this doesn't fix things where the part after the / needs more than one capitalized letter as shown by Europe/isle_of_man.

Otherwise we could scan the filesystem (/usr/share/zoneinfo) but that doesn't have to match what Go is using. If we have that list we could do a case insensitive match against that list.

@kennytm
Copy link
Contributor

kennytm commented Oct 27, 2021

if the time zone string contains one or more / you could just use regexp to upper-casing (^|[-_/])[a-zA-Z] and lower-casing the other characters. for the current tzdb will only miss:

  • Antarctica/DumontDUrville
  • America/Port-au-Prince
  • America/Port_of_Spain
  • everything in Etc/*, like Etc/GMT-14

Plus the following time zone aliases:

  • Africa/Dar_es_Salaam -> Nairobi
  • America/Knox_IN -> Indiana/Knox
  • America/Argentina/ComodRivadavia -> Catamarca
  • Antarctica/McMurdo -> ../Pacific/Auckland
  • Australia/ACT -> Sydney
  • Australia/LHI -> Lord_Howe
  • Australia/NSW -> Sydney
  • Brazil/DeNoronha -> ../America/Noronha
  • Chile/EasterIsland -> ../Pacific/Easter
  • Europe/Isle_of_Man -> London
  • Mexico/BajaNorte -> ../America/Tijuana
  • Mexico/BajaSur -> ../America/Mazatlan
  • everything in US/* and SystemV/*

@morgo
Copy link
Contributor Author

morgo commented Nov 23, 2021

I have a suggestion for an alternative solution:

Have a list of timezones (the list itself doesn't change that much) and if the TZ is unknown, use strings.EqualFold to compare it to known correct timezones. If it matches, substitute the correct name.

@dveeden
Copy link
Contributor

dveeden commented Nov 25, 2021

@morgo like this?

package main

import (
	"bytes"
	"fmt"
	"io/fs"
	"os"
	"path/filepath"
)

func main() {
	tzPath := "/usr/share/zoneinfo"
	tzMarker := []byte("TZif2")

	err := filepath.Walk(tzPath, func(path string, info fs.FileInfo, err error) error {
		if info.Mode().IsRegular() {
			f, err := os.Open(path)
			if err != nil {
				panic(err)
			}
			mark := make([]byte, 5)
			f.Read(mark)

			if bytes.Compare(mark, tzMarker) == 0 {
				fmt.Printf("%s\n", path[len(tzPath)+1:])
			}
			f.Close()
		}

		return nil
	})
	if err != nil {
		panic(err)
	}
}
[dvaneeden@dve-carbon tmp]$ go run tz.go | head
Africa/Abidjan
Africa/Accra
Africa/Addis_Ababa
Africa/Algiers
Africa/Asmara
Africa/Asmera
Africa/Bamako
Africa/Bangui
Africa/Banjul
Africa/Bissau
signal: broken pipe

Want to do this at build time, runtime or manually?

Another way to do this:

$ mysql_tzinfo_to_sql /usr/share/zoneinfo/ 2> /dev/null | awk '/INSERT INTO time_zone_name/ { print substr($7,3,length($7)-4)}' | head
Africa/Abidjan
Africa/Accra
Africa/Addis_Ababa
Africa/Algiers
Africa/Asmara
Africa/Asmera
Africa/Bamako
Africa/Bangui
Africa/Banjul
Africa/Bissau

@morgo
Copy link
Contributor Author

morgo commented Nov 26, 2021

@morgo like this?

Yes.

Want to do this at build time, runtime or manually?

I am fine with manually, but lets see what others say.

@kennytm
Copy link
Contributor

kennytm commented Nov 26, 2021

The tzMarker should be "TZif" not "TZif2". Versions 3 and 4 of those files do exist.

@mjonss
Copy link
Contributor

mjonss commented Feb 17, 2022

I think we should close this bug as Won't fix, since the impact is in my opinion low.

If we want to invest in fixing this, then I agree with the solution of at server start, listing all time zone files in the four different paths, create a map from lower case version -> original names and use for lookup, with some details below.

When it comes to how go looks up time zone files, it depends on the platform, in the zoneSources variable, which is not exported, but we could just iterate these paths:

// Many systems use /usr/share/zoneinfo, Solaris 2 has
// /usr/share/lib/zoneinfo, IRIX 6 has /usr/lib/locale/TZ.
var [zoneSources](https://cs.opensource.google/go/go/+/refs/tags/go1.17.7:src/time/zoneinfo_unix.go;bpv=1;bpt=1;l=22?gsn=zoneSources&gs=kythe%3A%2F%2Fgolang.org%3Flang%3Dgo%3Fpath%3Dtime%23var%2520zoneSources) = [][string](https://cs.opensource.google/go/go/+/refs/tags/go1.17.7:src/time/zoneinfo_unix.go;bpv=1;bpt=1;l=22?gsn=string&gs=kythe%3A%2F%2Fgolang.org%3Flang%3Dgo%23string%2523builtin){
	"/usr/share/zoneinfo/",
	"/usr/share/lib/zoneinfo/",
	"/usr/lib/locale/TZ/",
	runtime.GOROOT() + "/lib/time/zoneinfo.zip",
}

Also in this case it is good that we do not include the time/tzdata pkg, which would embed a zip-file in the binary with a static set of time zones as of the specific go version used for compiling the tidb-server binary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority/P4 Minor issue, awaiting more evidence before prioritizing type/compatibility
Projects
No open projects
SIG Runtime Kanban
  
Backlog: Low Priority
Development

No branches or pull requests

4 participants