Permalink
Browse files

Default to using a consistent, strict sql_mode. Fixes #31

Unless overridden by --connect-options, Skeema will now use the following
session-level sql_mode for its connections:

ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION

This provides a consistent baseline for behavior, regardless of what global
default the server is configured with. This is especially helpful for older
versions of MySQL, which don't default to strict mode. Additionally, this avoids
problems with servers that use ANSI_QUOTES, which breaks Skeema's behavior.

This commit also prevents using --connect-options to explicitly enable
ANSI_QUOTES, or the combination ANSI mode which includes it.
  • Loading branch information...
evanelias committed Jul 11, 2018
1 parent 0010a2e commit 4985a70e4bb31336c19ed01c123476d91a4efed9
Showing with 21 additions and 10 deletions.
  1. +7 −1 dir.go
  2. +5 −2 dir_test.go
  3. +9 −7 doc/options.md
View
8 dir.go
@@ -326,19 +326,25 @@ func (dir *Dir) InstanceDefaultParams() (string, error) {
if err != nil {
return "", err
}
v := url.Values{}
// Set overridable options
v.Set("timeout", "5s")
v.Set("readTimeout", "5s")
v.Set("writeTimeout", "5s")
v.Set("sql_mode", "'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'")
// Set values from connect-options
for name, value := range options {
if banned[strings.ToLower(name)] {
return "", fmt.Errorf("connect-options is not allowed to contain %s", name)
}
// Special case: never allow ANSI or ANSI_QUOTES in sql_mode, since this alters
// how identifiers are escaped in SHOW CREATE TABLES, utterly breaking Skeema
if strings.ToLower(name) == "sql_mode" && strings.Contains(strings.ToLower(value), "ansi") {
return "", fmt.Errorf("Skeema does not support use of the ANSI_QUOTES sql_mode")
}
v.Set(name, value)
}
View
@@ -3,6 +3,7 @@ package main
import (
"net/url"
"reflect"
"strings"
"testing"
"github.com/skeema/mybase"
@@ -103,6 +104,7 @@ func TestInstanceDefaultParams(t *testing.T) {
}
assertDefaultParams := func(connectOptions, expected string) {
t.Helper()
dir := getDir(connectOptions)
if parsed, err := url.ParseQuery(expected); err != nil {
t.Fatalf("Bad expected value \"%s\": %s", expected, err)
@@ -116,14 +118,14 @@ func TestInstanceDefaultParams(t *testing.T) {
t.Errorf("Expected connect-options=\"%s\" to yield default params \"%s\", instead found \"%s\"", connectOptions, expected, actual)
}
}
baseDefaults := "interpolateParams=true&foreign_key_checks=0&timeout=5s&writeTimeout=5s&readTimeout=5s"
baseDefaults := "interpolateParams=true&foreign_key_checks=0&timeout=5s&writeTimeout=5s&readTimeout=5s&sql_mode=%27ONLY_FULL_GROUP_BY%2CSTRICT_TRANS_TABLES%2CNO_ZERO_IN_DATE%2CNO_ZERO_DATE%2CERROR_FOR_DIVISION_BY_ZERO%2CNO_ENGINE_SUBSTITUTION%27"
expectParams := map[string]string{
"": baseDefaults,
"foo='bar'": baseDefaults + "&foo=%27bar%27",
"bool=true,quotes='yes,no'": baseDefaults + "&bool=true&quotes=%27yes,no%27",
`escaped=we\'re ok`: baseDefaults + "&escaped=we%5C%27re ok",
`escquotes='we\'re still quoted',this=that`: baseDefaults + "&escquotes=%27we%5C%27re still quoted%27&this=that",
"ok=1,writeTimeout=12ms": "interpolateParams=true&foreign_key_checks=0&timeout=5s&writeTimeout=12ms&readTimeout=5s&ok=1",
"ok=1,writeTimeout=12ms": strings.Replace(baseDefaults, "writeTimeout=5s", "writeTimeout=12ms&ok=1", 1),
}
for connOpts, expected := range expectParams {
assertDefaultParams(connOpts, expected)
@@ -133,6 +135,7 @@ func TestInstanceDefaultParams(t *testing.T) {
"totally_benign=1,allowAllFiles=true",
"FOREIGN_key_CHECKS='on'",
"bad_parse",
"lock_wait_timeout=60,sql_mode='STRICT_ALL_TABLES,ANSI,ALLOW_INVALID_DATES',wait_timeout=86400",
}
for _, connOpts := range expectError {
dir := getDir(connOpts)
View
@@ -165,33 +165,35 @@ On each individual database instance, only one DDL operation will be run at a ti
Commands | *all*
--- | :---
**Default** | *empty string*
**Default** | *empty string* (see below)
**Type** | string
**Restrictions** | none
This option stores a comma-separated list of session variables to set upon connecting to the database. For example, a value of `wait_timeout=86400,innodb_lock_wait_timeout=1,lock_wait_timeout=60` would set these three MySQL variables, at the session level, for connections made by Skeema.
Any string-valued variables must have their values wrapped in single-quotes. Take extra care to nest or escape quotes properly in your shell if supplying connect-options on the command-line. For example, `--connect-options="lock_wait_timeout=60,sql_mode='STRICT_ALL_TABLES,ALLOW_INVALID_DATES'"`
Additionally the following MySQL variables *cannot* be set by this option, since it would interfere with Skeema's internal operations:
The following MySQL variables *cannot* be set by this option, since it would interfere with Skeema's internal operations:
* `autocommit` -- cannot be disabled in Skeema
* `foreign_key_checks` -- see Skeema's own [foreign-key-checks](#foreign_key_checks) option to manipulate this
Aside from these, any legal MySQL session variable may be set.
Aside from these two, any legal MySQL session variable may be set.
This option only affects connections made *directly* by Skeema. If you are using an external tool via [alter-wrapper](#alter-wrapper) or [ddl-wrapper](#ddl-wrapper), you will also need to configure that tool to set options appropriately. Skeema's `{CONNOPTS}` variable can help avoid redundancy here; for example, if configuring pt-online-schema-change, you could include `--set-vars {CONNOPTS}` on the command-line to pass the same configured options dynamically.
If you do not override `sql_mode` in [connect-options](#connect-options), Skeema will default to using a session-level value of `'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'`. This provides a consistent strict-mode baseline for Skeema's behavior, regardless of what the server global default is set to.
In addition to setting MySQL session variables, you may also set any of these special variables which affect client-side behavior at the internal driver/protocol level:
* `charset=string` -- Character set used for client-server interaction
* `collation=string` -- Collation used for client-server interaction
* `maxAllowedPacket=int` -- Max allowed packet size, in bytes
* `readTimeout=duration` -- Read timeout; the value must be a float with a unit suffix ("ms" or "s")
* `timeout=duration` -- Connection timeout; the value must be a float with a unit suffix ("ms" or "s")
* `writeTimeout=duration` -- Write timeout; the value must be a float with a unit suffix ("ms" or "s")
* `readTimeout=duration` -- Read timeout; the value must be a float with a unit suffix ("ms" or "s"); default 5s
* `timeout=duration` -- Connection timeout; the value must be a float with a unit suffix ("ms" or "s"); default 5s
* `writeTimeout=duration` -- Write timeout; the value must be a float with a unit suffix ("ms" or "s"); default 5s
All special variables are case-sensitive. Unlike session variables, their values should never be wrapped in quotes. These special non-MySQL-variables are automatically stripped from `{CONNOPTS}`, so they won't be passed through to tools that don't understand them.
All six of these special variables are case-sensitive. Unlike session variables, their values should never be wrapped in quotes. These special non-MySQL variables are automatically stripped from `{CONNOPTS}`, so they won't be passed through to tools that don't understand them.
### ddl-wrapper

0 comments on commit 4985a70

Please sign in to comment.