Permalink
Browse files

Unified handling of table DDL and database DDL

Previously, database DDL (CREATE DATABASE, ALTER DATABASE) was handled as
a special-case, with a distinct code path. This commit refactors DDL display
and execution to be generic, regardless of object type.

A few implications of this change:

* The --ddl-wrapper option now also applies to database DDL. A few new
  template variables have been added to help handle this case, such as
  {CLASS} which will be either "TABLE" or "DATABASE".

* If an unsafe statement is not permitted, database DDL will be skipped
  along with table DDL. (Previously, the database DDL would still be
  displayed or executed, and only the table DDL would be skipped.)

* Support for other object types (besides tables and databases) will be
  easier to implement in the future, especially in the applier package.
  • Loading branch information...
evanelias committed Dec 29, 2018
1 parent 901df30 commit a8180e995ea97a3e6c6306c87c502651319928e2

Some generated files are not rendered by default. Learn more.

Oops, something went wrong.
@@ -4,7 +4,6 @@ package applier

import (
"context"
"fmt"
"strings"

log "github.com/sirupsen/logrus"
@@ -44,31 +43,6 @@ func Worker(ctx context.Context, targetGroups <-chan TargetGroup, results chan<-
diff := tengo.NewSchemaDiff(t.SchemaFromInstance, t.SchemaFromDir)
var targetStmtCount int

if diff.SchemaDDL != "" {
printer.syncPrintf(t.Instance, "", "%s;\n", diff.SchemaDDL)
targetStmtCount++
result.Differences = true
if !dryRun {
var schemaDDLErr error
if strings.HasPrefix(diff.SchemaDDL, "CREATE DATABASE") && t.SchemaFromInstance == nil {
_, schemaDDLErr = t.Instance.CreateSchema(schemaName, t.SchemaFromDir.CharSet, t.SchemaFromDir.Collation)
} else if strings.HasPrefix(diff.SchemaDDL, "ALTER DATABASE") {
schemaDDLErr = t.Instance.AlterSchema(t.SchemaFromInstance.Name, t.SchemaFromDir.CharSet, t.SchemaFromDir.Collation)
} else {
return fmt.Errorf("Refusing to run unexpectedly-generated schema-level DDL: %s", diff.SchemaDDL)
}
if schemaDDLErr != nil {
log.Errorf("Error running DDL on %s %s: %s", t.Instance, schemaName, schemaDDLErr)
skipped := len(diff.TableDiffs) + 1
result.SkipCount += skipped
if skipped > 1 {
log.Warnf("Skipping %d remaining operations for %s %s due to previous error", skipped-1, t.Instance, schemaName)
}
continue
}
}
}

if t.Dir.Config.GetBool("verify") && len(diff.TableDiffs) > 0 && !brief {
if err := VerifyDiff(diff, t); err != nil {
return err
@@ -86,52 +60,46 @@ func Worker(ctx context.Context, targetGroups <-chan TargetGroup, results chan<-
mods.Flavor = t.Instance.Flavor()
}

// Build DDLStatements for each TableDiff, handling pre-execution errors
// Build DDLStatements for each ObjectDiff, handling pre-execution errors
// accordingly
ddls := make([]*DDLStatement, 0, len(diff.TableDiffs))
for i, tableDiff := range diff.TableDiffs {
ddl, err := NewDDLStatement(tableDiff, mods, t)
objDiffs := diff.ObjectDiffs()
ddls := make([]*DDLStatement, 0, len(objDiffs))
for _, objDiff := range objDiffs {
ddl, err := NewDDLStatement(objDiff, mods, t)
if ddl == nil && err == nil {
continue // Skip entirely if mods made the statement a noop
}
targetStmtCount++
result.Differences = true

if unsupportedErr, ok := err.(*tengo.UnsupportedDiffError); ok {
if err == nil {
ddls = append(ddls, ddl)
} else if unsupportedErr, ok := err.(*tengo.UnsupportedDiffError); ok {
result.UnsupportedCount++
log.Warnf("Skipping table %s: unable to generate ALTER TABLE due to use of unsupported features. Use --debug for more information.", unsupportedErr.Name)
log.Warnf("Skipping %s %s: unable to generate DDL due to use of unsupported features. Use --debug for more information.", unsupportedErr.ObjectType, unsupportedErr.Name)
DebugLogUnsupportedDiff(unsupportedErr)
continue
} else if err != nil {
skipped := len(diff.TableDiffs) - i
result.SkipCount += skipped
} else {
result.SkipCount += len(objDiffs)
log.Errorf(err.Error())
if skipped > 1 {
log.Warnf("Skipping %d remaining operations for %s %s due to previous error", skipped-1, t.Instance, schemaName)
if len(objDiffs) > 1 {
log.Warnf("Skipping %d additional operations for %s %s due to previous error", len(objDiffs)-1, t.Instance, schemaName)
}
continue TargetsInGroup
}

if dryRun {
printer.syncPrintf(t.Instance, schemaName, "%s\n", ddl.String())
} else {
// IMPORTANT: only append to ddls if NOT dry-run. Do not move this out of
// this conditional!
ddls = append(ddls, ddl)
}
}

// Execute DDL. (ddls will be empty if dryRun, due to conditional above)
// Print DDL; if not dry-run, execute it
for i, ddl := range ddls {
printer.syncPrintf(t.Instance, schemaName, "%s\n", ddl.String())
if err := ddl.Execute(); err != nil {
log.Errorf("Error running DDL on %s %s: %s", t.Instance, schemaName, err)
skipped := len(ddls) - i
result.SkipCount += skipped
if skipped > 1 {
log.Warnf("Skipping %d remaining operations for %s %s due to previous error", skipped-1, t.Instance, schemaName)
printer.printDDL(ddl)
if !dryRun {
if err := ddl.Execute(); err != nil {
log.Errorf("Error running DDL on %s %s: %s", t.Instance, schemaName, err)
skipped := len(ddls) - i
result.SkipCount += skipped
if skipped > 1 {
log.Warnf("Skipping %d remaining operations for %s %s due to previous error", skipped-1, t.Instance, schemaName)
}
break
}
break
}
}

@@ -190,7 +158,7 @@ func StatementModifiersForDir(dir *fs.Dir) (mods tengo.StatementModifiers, err e
return
}

// DebugLogUnsupportedDiff logs (at Debug level) the reason why a table is
// DebugLogUnsupportedDiff logs (at Debug level) the reason why an object is
// unsupported for diff/alter operations.
func DebugLogUnsupportedDiff(err *tengo.UnsupportedDiffError) {
for _, line := range strings.Split(err.ExtendedError(), "\n") {
@@ -28,20 +28,26 @@ type DDLStatement struct {
// an error constructing the statement (mods disallowing destructive DDL,
// invalid variable interpolation in --alter-wrapper, etc), the DDLStatement
// pointer will be nil, and a non-nil error will be returned.
func NewDDLStatement(diff *tengo.TableDiff, mods tengo.StatementModifiers, target *Target) (ddl *DDLStatement, err error) {
func NewDDLStatement(diff tengo.ObjectDiff, mods tengo.StatementModifiers, target *Target) (ddl *DDLStatement, err error) {
ddl = &DDLStatement{
instance: target.Instance,
schemaName: target.SchemaFromDir.Name,
}

// Obtain table name, and possibly its current size (only if we actually need it)
var tableName string
var isTable bool
var tableSize int64
if diff.Type == tengo.TableDiffCreate { // current size is inherently 0 for CREATE
tableName = diff.To.Name
} else { // ALTER or DROP
tableName = diff.From.Name
if anyOptChanged(target, "safe-below-size", "alter-wrapper-min-size") || wrapperUsesSize(target, "alter-wrapper", "ddl-wrapper") {

switch diff := diff.(type) {
case *tengo.DatabaseDiff:
// Don't run database-level DDL in a schema; not even possible for CREATE
// DATABASE anyway
ddl.schemaName = ""

case *tengo.TableDiff:
isTable = true
// Obtain table size only if actually needed
needSize := anyOptChanged(target, "safe-below-size", "alter-wrapper-min-size") || wrapperUsesSize(target, "alter-wrapper", "ddl-wrapper")
if diff.DiffType() != tengo.DiffTypeCreate && needSize {
if tableSize, err = ddl.getTableSize(target, diff.From); err != nil {
return nil, err
}
@@ -54,14 +60,14 @@ func NewDDLStatement(diff *tengo.TableDiff, mods tengo.StatementModifiers, targe
if err != nil {
return nil, err
}
if tableSize < int64(safeBelowSize) {
if isTable && tableSize < int64(safeBelowSize) {
mods.AllowUnsafe = true
log.Debugf("Allowing unsafe operations for table %s: size=%d < safe-below-size=%d", tableName, tableSize, safeBelowSize)
log.Debugf("Allowing unsafe operations for table %s: size=%d < safe-below-size=%d", diff.ObjectName(), tableSize, safeBelowSize)
}

// Options may indicate some/all DDL gets executed by shelling out to another program.
wrapper := target.Dir.Config.Get("ddl-wrapper")
if diff.Type == tengo.TableDiffAlter && target.Dir.Config.Changed("alter-wrapper") {
if isTable && diff.DiffType() == tengo.DiffTypeAlter && target.Dir.Config.Changed("alter-wrapper") {
minSize, err := target.Dir.Config.GetBytes("alter-wrapper-min-size")
if err != nil {
return nil, err
@@ -75,15 +81,15 @@ func NewDDLStatement(diff *tengo.TableDiff, mods tengo.StatementModifiers, targe
// external OSC tool for large tables, without risk of ALGORITHM or LOCK
// clauses breaking expectations of the OSC tool.
if minSize > 0 {
log.Debugf("Using alter-wrapper for table %s: size=%d >= alter-wrapper-min-size=%d", tableName, tableSize, minSize)
log.Debugf("Using alter-wrapper for table %s: size=%d >= alter-wrapper-min-size=%d", diff.ObjectName(), tableSize, minSize)
if mods.AlgorithmClause != "" || mods.LockClause != "" {
log.Debug("Ignoring --alter-algorithm and --alter-lock for generating DDL for alter-wrapper")
mods.AlgorithmClause = ""
mods.LockClause = ""
}
}
} else {
log.Debugf("Skipping alter-wrapper for table %s: size=%d < alter-wrapper-min-size=%d", tableName, tableSize, minSize)
log.Debugf("Skipping alter-wrapper for table %s: size=%d < alter-wrapper-min-size=%d", diff.ObjectName(), tableSize, minSize)
}
}

@@ -100,7 +106,7 @@ func NewDDLStatement(diff *tengo.TableDiff, mods tengo.StatementModifiers, targe
}

// If adding foreign key constraints, use foreign_key_checks=1 if requested
if wrapper == "" && diff.Type == tengo.TableDiffAlter &&
if wrapper == "" && isTable && diff.DiffType() == tengo.DiffTypeAlter &&
strings.Contains(ddl.stmt, "ADD CONSTRAINT") &&
strings.Contains(ddl.stmt, "FOREIGN KEY") &&
target.Dir.Config.GetBool("foreign-key-checks") {
@@ -109,8 +115,7 @@ func NewDDLStatement(diff *tengo.TableDiff, mods tengo.StatementModifiers, targe

// Apply wrapper if relevant
if wrapper != "" {
var clauses, socket, port, connOpts string
clauses, _ = diff.Clauses(mods)
var socket, port, connOpts string
if ddl.instance.SocketPath != "" {
socket = ddl.instance.SocketPath
} else {
@@ -128,14 +133,22 @@ func NewDDLStatement(diff *tengo.TableDiff, mods tengo.StatementModifiers, targe
"PASSWORD": target.Dir.Config.Get("password"),
"ENVIRONMENT": target.Dir.Config.Get("environment"),
"DDL": ddl.stmt,
"TABLE": tableName,
"CLAUSES": "", // filled in below only for tables
"NAME": diff.ObjectName(),
"TABLE": "", // filled in below only for tables
"SIZE": strconv.FormatInt(tableSize, 10),
"CLAUSES": clauses,
"TYPE": diff.TypeString(),
"TYPE": diff.DiffType().String(),
"CLASS": strings.ToUpper(diff.ObjectType()),
"CONNOPTS": connOpts,
"DIRNAME": target.Dir.BaseName(),
"DIRPATH": target.Dir.Path,
}
if isTable {
td := diff.(*tengo.TableDiff)
variables["CLAUSES"], _ = td.Clauses(mods)
variables["TABLE"] = variables["NAME"]
}

if ddl.shellOut, err = util.NewInterpolatedShellOut(wrapper, variables); err != nil {
errorText := fmt.Sprintf("A fatal error occurred with pre-processing a DDL statement: %s.", err)
return nil, errors.New(errorText)
@@ -52,7 +52,7 @@ func (s ApplierIntegrationSuite) TestNewDDLStatement(t *testing.T) {
"password": s.d[0].Instance.Password,
"debug": "1",
"allow-unsafe": "1",
"ddl-wrapper": "/bin/echo ddl-wrapper {SCHEMA}.{TABLE} {TYPE}",
"ddl-wrapper": "/bin/echo ddl-wrapper {SCHEMA}.{NAME} {TYPE} {CLASS}",
"alter-wrapper": "/bin/echo alter-wrapper {SCHEMA}.{TABLE} {TYPE} {CLAUSES}",
"alter-wrapper-min-size": "1",
"alter-algorithm": "INPLACE",
@@ -80,18 +80,19 @@ func (s ApplierIntegrationSuite) TestNewDDLStatement(t *testing.T) {
}

sd := tengo.NewSchemaDiff(target.SchemaFromInstance, target.SchemaFromDir)
if len(sd.TableDiffs) != 4 {
// modifications in ddlstatement.sql should have yielded 4 diffs: one drop
// table, one create table, and two alter tables (one to a table with rows
// and one to a table without rows)
t.Fatalf("Expected 4 table diffs, instead found %d", len(sd.TableDiffs))
objDiffs := sd.ObjectDiffs()
if len(objDiffs) != 5 {
// modifications in ddlstatement.sql should have yielded 5 diffs: one alter
// database, one drop table, one create table, and two alter tables (one to
// a table with rows and one to a table without rows)
t.Fatalf("Expected 5 object diffs, instead found %d %#v", len(objDiffs), objDiffs)
}

mods := tengo.StatementModifiers{AllowUnsafe: true}
if !is55 {
mods.LockClause, mods.AlgorithmClause = "NONE", "INPLACE"
}
for _, diff := range sd.TableDiffs {
for _, diff := range objDiffs {
ddl, err := NewDDLStatement(diff, mods, target)
if err != nil {
t.Errorf("Unexpected DDLStatement error: %s", err)
@@ -100,33 +101,44 @@ func (s ApplierIntegrationSuite) TestNewDDLStatement(t *testing.T) {
t.Fatalf("Expected this configuration to result in all DDLs being shellouts, but %v is not", ddl)
}
var expected string
switch diff.Type {
case tengo.TableDiffAlter:
if diff.To.Name == "rollups" {
// no rows, so ddl-wrapper used. verify the statement separately.
expected = "/bin/echo ddl-wrapper analytics.rollups ALTER"
expectedStmt := "ALTER TABLE `rollups` ALGORITHM=INPLACE, LOCK=NONE, ADD COLUMN `value` bigint(20) DEFAULT NULL"
if is55 {
expectedStmt = "ALTER TABLE `rollups` ADD COLUMN `value` bigint(20) DEFAULT NULL"
}
if ddl.stmt != expectedStmt {
t.Errorf("Expected statement:\n%s\nActual statement:\n%s\n", expectedStmt, ddl.stmt)
}
} else if diff.To.Name == "pageviews" {
// has 1 row, so alter-wrapper used. verify the execution separately to
// sanity-check the quoting rules.
expected = "/bin/echo alter-wrapper analytics.pageviews ALTER 'ADD COLUMN `domain` varchar(40) NOT NULL DEFAULT '\"'\"'skeema.io'\"'\"''"
expectedOutput := "alter-wrapper analytics.pageviews ALTER ADD COLUMN `domain` varchar(40) NOT NULL DEFAULT 'skeema.io'\n"
if actualOutput, err := ddl.shellOut.RunCapture(); err != nil || actualOutput != expectedOutput {
t.Errorf("Expected output:\n%sActual output:\n%sErr:\n%v\n", expectedOutput, actualOutput, err)
switch diff := diff.(type) {
case *tengo.DatabaseDiff:
expected = "/bin/echo ddl-wrapper .analytics ALTER DATABASE"
if ddl.schemaName != "" {
t.Errorf("Unexpected DDLStatement.schemaName: %s", ddl.schemaName)
}
case *tengo.TableDiff:
if ddl.schemaName != "analytics" {
t.Errorf("Unexpected DDLStatement.schemaName: %s", ddl.schemaName)
}
switch diff.DiffType() {
case tengo.DiffTypeAlter:
if diff.To.Name == "rollups" {
// no rows, so ddl-wrapper used. verify the statement separately.
expected = "/bin/echo ddl-wrapper analytics.rollups ALTER TABLE"
expectedStmt := "ALTER TABLE `rollups` ALGORITHM=INPLACE, LOCK=NONE, ADD COLUMN `value` bigint(20) DEFAULT NULL"
if is55 {
expectedStmt = "ALTER TABLE `rollups` ADD COLUMN `value` bigint(20) DEFAULT NULL"
}
if ddl.stmt != expectedStmt {
t.Errorf("Expected statement:\n%s\nActual statement:\n%s\n", expectedStmt, ddl.stmt)
}
} else if diff.To.Name == "pageviews" {
// has 1 row, so alter-wrapper used. verify the execution separately to
// sanity-check the quoting rules.
expected = "/bin/echo alter-wrapper analytics.pageviews ALTER 'ADD COLUMN `domain` varchar(40) NOT NULL DEFAULT '\"'\"'skeema.io'\"'\"''"
expectedOutput := "alter-wrapper analytics.pageviews ALTER ADD COLUMN `domain` varchar(40) NOT NULL DEFAULT 'skeema.io'\n"
if actualOutput, err := ddl.shellOut.RunCapture(); err != nil || actualOutput != expectedOutput {
t.Errorf("Expected output:\n%sActual output:\n%sErr:\n%v\n", expectedOutput, actualOutput, err)
}
} else {
t.Fatalf("Unexpected AlterTable for %s; perhaps test fixture changed without updating this test?", diff.To.Name)
}
} else {
t.Fatalf("Unexpected AlterTable for %s; perhaps test fixture changed without updating this test?", diff.To.Name)
case tengo.DiffTypeDrop:
expected = "/bin/echo ddl-wrapper analytics.widget_counts DROP TABLE"
case tengo.DiffTypeCreate:
expected = "/bin/echo ddl-wrapper analytics.activity CREATE TABLE"
}
case tengo.TableDiffDrop:
expected = "/bin/echo ddl-wrapper analytics.widget_counts DROP"
case tengo.TableDiffCreate:
expected = "/bin/echo ddl-wrapper analytics.activity CREATE"
}
if ddl.shellOut.Command != expected {
t.Errorf("Expected shellout:\n%s\nActual shellout:\n%s\n", expected, ddl.shellOut.Command)
Oops, something went wrong.

0 comments on commit a8180e9

Please sign in to comment.