Permalink
Browse files

Strict mode usage user-friendliness improvements

Since v1.0.5, Skeema has defaulted to using strict-mode settings for its
sessions: innodb_strict_mode and a strict sql_mode. This is generally a good
practice, but can cause confusing errors in Skeema when operating on databases
that are not strict-mode compliant. The most common conflicts are ROW_FORMAT
clauses (see #40) as well as zero-date defaults for date/datetime/timestamp
columns.

This commit makes two improvements regarding user-friendliness:

* `skeema init` now detects if at least one table isn't strict-mode compliant,
  and automatically puts `connect-options` into the .skeema file to disable
  strict-mode when interacting with this instance.

* `skeema diff` and `skeema push` now provide explanatory text when the a .sql
  file cannot be executed for reasons that may relate to strict-mode.
  • Loading branch information...
evanelias committed Nov 26, 2018
1 parent 31f47f0 commit 9bebd1d6e975d5a2970a63ad49c0ab2bd0af8bfa
Showing with 46 additions and 4 deletions.
  1. +7 −1 applier/target.go
  2. +18 −1 cmd_init.go
  3. +1 −1 doc/options.md
  4. +11 −0 skeema_cmd_test.go
  5. +9 −1 skeema_test.go
@@ -1,6 +1,8 @@
package applier
import (
"strings"
log "github.com/sirupsen/logrus"
"github.com/skeema/skeema/fs"
"github.com/skeema/skeema/workspace"
@@ -125,13 +127,17 @@ func targetsForLogicalSchema(logicalSchema *fs.LogicalSchema, dir *fs.Dir, insta
}
for _, stmtErr := range statementErrors {
log.Error(stmtErr.Error())
if (strings.Contains(stmtErr.Error(), "Error 1031") || strings.Contains(stmtErr.Error(), "Error 1067")) && !dir.Config.Changed("connect-options") {
log.Info("This may be caused by Skeema's default usage of strict-mode settings. To disable strict-mode, add this to a .skeema file:")
log.Info("connect-options=\"innodb_strict_mode=0,sql_mode='ONLY_FULL_GROUP_BY,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'\"\n")
}
}
if len(statementErrors) > 0 {
noun := "errors"
if len(statementErrors) == 1 {
noun = "error"
}
log.Warnf("Skipping %s due to %d SQL %s", dir, len(statementErrors), noun)
log.Warnf("Skipping %s due to %d SQL %s\n", dir, len(statementErrors), noun)
return nil, len(instances)
}
@@ -124,6 +124,17 @@ func InitHandler(cfg *mybase.Config) error {
hostOptionFile.SetOptionValue("", "default-collation", schemas[0].Collation)
}
// By default, Skeema normally connects using strict sql_mode as well as
// innodb_strict_mode=1; see InstanceDefaultParams() in fs/dir.go. If existing
// tables aren't recreatable with those settings though, disable them.
var nonStrictWarning string
if !cfg.OnCLI("connect-options") {
if compliant, err := inst.StrictModeCompliant(schemas); err == nil && !compliant {
nonStrictWarning = fmt.Sprintf("Detected some tables are incompatible with strict-mode; setting relaxed connect-options in %s\n", hostOptionFile)
hostOptionFile.SetOptionValue(environment, "connect-options", "innodb_strict_mode=0,sql_mode='ONLY_FULL_GROUP_BY,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'")
}
}
// Write the option file
if err := hostOptionFile.Write(false); err != nil {
return NewExitValue(CodeCantCreate, "Unable to use directory %s: Unable to write to %s: %s", hostDir.Path, hostOptionFile.Path(), err)
@@ -137,7 +148,13 @@ func InitHandler(cfg *mybase.Config) error {
if !separateSchemaSubdir {
suffix = "; skipping schema-level subdirs"
}
log.Infof("%s host dir %s for %s%s\n", verb, hostDir.Path, inst, suffix)
if nonStrictWarning == "" {
suffix += "\n"
}
log.Infof("%s host dir %s for %s%s", verb, hostDir.Path, inst, suffix)
if nonStrictWarning != "" {
log.Warn(nonStrictWarning)
}
// Iterate over the schemas. For each one, create a dir with .skeema and *.sql files
for _, s := range schemas {
@@ -197,7 +197,7 @@ Aside from the above list, 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. Similarly, `innodb_strict_mode` is enabled by default for Skeema's sessions, but may be overridden to disable if desired.
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. Similarly, `innodb_strict_mode` is enabled by default for Skeema's sessions, but may be overridden to disable if desired. Note that `skeema init` will automatically set a non-strict [connect-options](#connect-options) in `.skeema` if at least one existing table is incompatible with strict settings (e.g., use of a zero-date default, or an unsupported ROW_FORMAT).
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:
@@ -88,6 +88,17 @@ func (s SkeemaIntegrationSuite) TestInitHandler(t *testing.T) {
}
}
// Test successful init on a schema that isn't strict-mode compliant
s.dbExecWithParams(t, "product", "sql_mode=%27NO_ENGINE_SUBSTITUTION%27", "ALTER TABLE posts MODIFY COLUMN created_at datetime NOT NULL DEFAULT '0000-00-00 00:00:00'")
cfg = s.handleCommand(t, CodeSuccess, ".", "skeema init -h %s -P %d --dir nonstrict", s.d.Instance.Host, s.d.Instance.Port)
if dir, err = fs.ParseDir("nonstrict", cfg); err != nil {
t.Fatalf("Unexpected error from ParseDir: %s", err)
}
value, _ := dir.OptionFile.OptionValue("connect-options")
if !strings.Contains(value, "innodb_strict_mode=0") {
t.Errorf("Expected non-strict-compliant schema to use relaxed connect-options; instead found connect-options=%s", value)
}
// init should fail if a parent dir has an invalid .skeema file
fs.MakeTestDirectory(t, "hasbadoptions")
fs.WriteTestFile(t, "hasbadoptions/.skeema", "invalid file will not parse")
@@ -382,7 +382,15 @@ func (s *SkeemaIntegrationSuite) cleanData(t *testing.T, sourceAfter ...string)
// something goes wrong, it is fatal to the current test.
func (s *SkeemaIntegrationSuite) dbExec(t *testing.T, schemaName, query string, args ...interface{}) {
t.Helper()
db, err := s.d.Connect(schemaName, "")
s.dbExecWithParams(t, schemaName, "", query, args...)
}
// dbExecWithOptions run the specified SQL DML or DDL in the specified schema,
// using the supplied URI-encoded session variables. If something goes wrong,
// it is fatal to the current test.
func (s *SkeemaIntegrationSuite) dbExecWithParams(t *testing.T, schemaName, params, query string, args ...interface{}) {
t.Helper()
db, err := s.d.Connect(schemaName, params)
if err != nil {
t.Fatalf("Unable to connect to DockerizedInstance: %s", err)
}

0 comments on commit 9bebd1d

Please sign in to comment.