Permalink
Browse files

diff/push: Refactor options involving unsafe operations

Previously, Skeema had separate options --allow-drop-table and
--allow-drop-column, which controlled generation/execution of specific DDL.
This commit now consolidates these into a single --allow-unsafe option. In
addition to handling table and column drops, it also catches additional risky
types of ALTER TABLE: lossy column type changes, column character set changes,
and storage engine changes.

The --allow-below-size option has been renamed --safe-below-size, to better
match its behavior ("consider operations safe on tables below this size").
  • Loading branch information...
evanelias committed Jan 17, 2017
1 parent 611b1b9 commit b1d438c45235edc39d28dd8c183a1a6508b9aeb2
View

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

Oops, something went wrong.
View
@@ -49,10 +49,10 @@ func clonePushOptionsToDiff() {
}
descRewrites := map[string]string{
- "allow-drop-table": "In output, include a DROP TABLE for any table without a corresponding *.sql file",
- "allow-drop-column": "In output, include DROP COLUMN clauses where appropriate",
- "alter-wrapper": "Output ALTER TABLEs as shell commands rather than just raw DDL; see manual for template vars",
- "all": "For dirs mapping to multiple instances or schemas, diff against all, not just the first",
+ "allow-unsafe": "Permit generating ALTER or DROP operations that are potentially destructive",
+ "alter-wrapper": "Output ALTER TABLEs as shell commands rather than just raw DDL; see manual for template vars",
+ "all": "For dirs mapping to multiple instances or schemas, diff against all, not just the first",
+ "safe-below-size": "Always permit generating destructive operations for tables below this size in bytes",
}
hiddenRewrites := map[string]bool{
"all": false,
View
@@ -96,11 +96,10 @@ func PullHandler(cfg *mycli.Config) error {
}
}
- // We're permissive of drops here since we don't ever actually execute the
- // generated statement! We just examine its type.
+ // We're permissive of unsafe operations here since we don't ever actually
+ // execute the generated statement! We just examine its type.
mods := tengo.StatementModifiers{
- AllowDropTable: true,
- AllowDropColumn: true,
+ AllowUnsafe: true,
}
// pull command updates next auto-increment value for existing table always
// if requested, or only if previously present in file otherwise
View
@@ -26,17 +26,16 @@ top of the file. If no environment name is supplied, the default is
cmd := mycli.NewCommand("push", summary, desc, PushHandler)
cmd.AddOption(mycli.BoolOption("verify", 0, true, "Test all generated ALTER statements on temp schema to verify correctness"))
- cmd.AddOption(mycli.BoolOption("allow-drop-table", 0, false, "Permit dropping any table that has no corresponding *.sql file"))
- cmd.AddOption(mycli.BoolOption("allow-drop-column", 0, false, "Permit dropping columns that are no longer present in *.sql file"))
+ cmd.AddOption(mycli.BoolOption("allow-unsafe", 0, false, "Permit running ALTER or DROP operations that are potentially destructive"))
cmd.AddOption(mycli.BoolOption("dry-run", 0, false, "Output DDL but don't run it; equivalent to `skeema diff`"))
- cmd.AddOption(mycli.BoolOption("first-only", 0, false, "For dirs mapping to multiple instances or schemas, just run against the first"))
+ cmd.AddOption(mycli.BoolOption("first-only", '1', false, "For dirs mapping to multiple instances or schemas, just run against the first per dir"))
cmd.AddOption(mycli.BoolOption("all", 0, false, "<overridden by diff command>").Hidden())
cmd.AddOption(mycli.StringOption("alter-wrapper", 'x', "", "External bin to shell out to for ALTER TABLE; see manual for template vars"))
cmd.AddOption(mycli.StringOption("alter-wrapper-min-size", 0, "0", "Ignore --alter-wrapper for tables smaller than this size in bytes"))
cmd.AddOption(mycli.StringOption("alter-lock", 0, "", `Apply a LOCK clause to all ALTER TABLEs (valid values: "NONE", "SHARED", "EXCLUSIVE")`))
cmd.AddOption(mycli.StringOption("alter-algorithm", 0, "", `Apply an ALGORITHM clause to all ALTER TABLEs (valid values: "INPLACE", "COPY")`))
cmd.AddOption(mycli.StringOption("ddl-wrapper", 'X', "", "Like --alter-wrapper, but applies to all DDL types (CREATE, DROP, ALTER)"))
- cmd.AddOption(mycli.StringOption("allow-below-size", 0, "0", "For tables under this size, act as if --allow-drop-table --allow-drop-column"))
+ cmd.AddOption(mycli.StringOption("safe-below-size", 0, "0", "Always permit destructive operations for tables below this size in bytes"))
cmd.AddOption(mycli.StringOption("concurrent-instances", 'c', "1", "Perform operations on this number of instances concurrently"))
cmd.AddArg("environment", "production", false)
CommandSuite.AddSubCommand(cmd)
@@ -205,8 +204,7 @@ func pushWorker(sps *sharedPushState) {
// Set configuration-dependent statement modifiers here inside the Target
// loop, since the config for these may var per dir!
- mods.AllowDropTable = t.Dir.Config.GetBool("allow-drop-table")
- mods.AllowDropColumn = t.Dir.Config.GetBool("allow-drop-column")
+ mods.AllowUnsafe = t.Dir.Config.GetBool("allow-unsafe")
mods.AlgorithmClause, err = t.Dir.Config.GetEnum("alter-algorithm", "INPLACE", "COPY", "DEFAULT")
if err != nil {
sps.setFatalError(err)
View
@@ -57,14 +57,13 @@ func NewDDLStatement(diff tengo.TableDiff, mods tengo.StatementModifiers, target
}
ddl.setErr(err)
- // If --allow-below-size option in use, enable additional statement modifiers
+ // If --safe-below-size option in use, enable additional statement modifier
// if the table's size is less than the supplied option value
- allowBelowSize, err := target.Dir.Config.GetBytes("allow-below-size")
+ safeBelowSize, err := target.Dir.Config.GetBytes("safe-below-size")
ddl.setErr(err)
- if ddl.Err == nil && tableSize < int64(allowBelowSize) {
- mods.AllowDropTable = true
- mods.AllowDropColumn = true
- log.Debugf("Allowing drops for table %s: size=%d < allow-below-size=%d", tableName, tableSize, allowBelowSize)
+ if ddl.Err == nil && tableSize < int64(safeBelowSize) {
+ mods.AllowUnsafe = true
+ log.Debugf("Allowing unsafe operations for table %s: size=%d < safe-below-size=%d", tableName, tableSize, safeBelowSize)
}
// Options may indicate some/all DDL gets executed by shelling out to another program.
View
@@ -38,20 +38,20 @@ Ordinarily, in between `skeema diff` and `skeema push`, you would want to make a
### Generate DDL from adding or removing files
-Similarly, if you add new .sql files with CREATE TABLE statements, these will be included in the output of `skeema diff` or execution of `skeema push`. Removing files translates to DROP TABLE, but only if --allow-drop-table is used.
+Similarly, if you add new .sql files with CREATE TABLE statements, these will be included in the output of `skeema diff` or execution of `skeema push`. Removing files translates to DROP TABLE, but only if --allow-unsafe is used.
```
vi product/comments.sql
skeema diff
rm product/tags.sql
skeema diff
-skeema diff --allow-drop-table
-skeema push --allow-drop-table
+skeema diff --allow-unsafe
+skeema push --allow-unsafe
```
-[![asciicast](https://asciinema.org/a/7nic9hyphd1m0xlprpzitbp56.png)](https://asciinema.org/a/7nic9hyphd1m0xlprpzitbp56)
+[![asciicast](https://asciinema.org/a/6n64ie4v1sberpnnosexnn4ua.png)](https://asciinema.org/a/6n64ie4v1sberpnnosexnn4ua)
-To make things more convenient and loosen the --allow-drop-table safety for smaller tables, try the [allow-below-size](options.md#allow-below-size) option. For example, putting `allow-below-size=10m` in ~/schemas/.skeema will remove the requirement of specifying --allow-drop-table when dropping any table under 10 megabytes in size. Or use `allow-below-size=1` to only loosen safeties for tables that have no rows. (Skeema always treats zero-row tables as size 0 bytes, as a special-case.)
+To aid in rapid development, you can configure Skeema to always allow dropping empty tables or small tables with the [safe-below-size](options.md#safe-below-size) option. For example, putting `safe-below-size=10m` in ~/schemas/.skeema will remove the requirement of specifying --allow-unsafe when dropping any table under 10 megabytes in size. Or use `safe-below-size=1` to only loosen safeties for tables that have no rows. (Skeema always treats zero-row tables as size 0 bytes, as a special-case.)
### Normalize format of CREATE TABLE files, and check for syntax errors
@@ -117,7 +117,7 @@ This example shows how to configure Skeema to use the following set of rules:
* Be fully permissive about dropping tables or columns in dev, regardless of table size
* Just use standard ALTERs, no online DDL or external OSC tool
* Any environment EXCEPT development
- * Only automatically allow dropping tables or columns for tables that have no rows. For any table with at least one row, force the user to supply --allow-drop-table or --allow-drop-column on the command-line to confirm when needed.
+ * Only automatically allow dropping tables or columns for tables that have no rows. For any table with at least one row, force the user to supply --allow-unsafe on the command-line to confirm when needed.
* For ALTERs, for any table 1GB or later, use pt-online-schema-change
* For ALTERs, for any table below 1GB, use MySQL 5.6 online DDL. (Some specific ALTERs will fail due to requiring offline DDL; in this case the user can supply --skip-alter-algorithm and/or --skip-alter-lock as needed.)
* Staging
@@ -130,13 +130,12 @@ alter-wrapper="/usr/local/bin/pt-online-schema-change --alter {CLAUSES} D={SCHEM
alter-wrapper-min-size=1g
alter-algorithm=inplace
alter-lock=none
-allow-below-size=1
+safe-below-size=1
[development]
host=localhost
socket=/var/lib/mysql/mysql.sock
-allow-drop-table
-allow-drop-column
+allow-unsafe
skip-alter-wrapper
skip-alter-algorithm
skip-alter-lock
View
@@ -33,16 +33,21 @@ Most Skeema commands need to perform intermediate operations in a scratch space
When operating on the temporary database, Skeema refuses to drop a table if it contains any rows, and likewise refuses to drop the database if any tables contain any rows. This prevents disaster if someone accidentally points [temp-schema](options.md#temp-schema) at a real schema, or accidentally starts storing real data in the temporary schema.
-#### Dropping tables and columns is prevented by default
+#### Destructive operations are prevented by default
-Destructive actions only occur when specifically requested. This prevents human error with running `skeema push` from an out-of-date repo working copy, as well as misinterpreting accidental attempts to rename tables or columns (both of which are not yet supported).
+Destructive operations only occur when specifically requested via the [allow-unsafe option](options.md#allow-unsafe). This prevents human error with running `skeema push` from an out-of-date repo working copy, as well as misinterpreting accidental attempts to rename tables or columns (both of which are not yet supported).
-* `skeema push` refuses to run any generated DROP TABLE statement, unless the [allow-drop-table option](options.md#allow-drop-table) is provided.
-* `skeema push` refuses to run any generated ALTER TABLE statement that drops columns, unless the [allow-drop-column option](options.md#allow-drop-column) is provided.
+The following operations are considered unsafe:
-`skeema diff` also provides the same two options, even though `skeema diff` never actually modifies tables regardless. These options are present so that `skeema diff` can serve as a safe dry-run that exactly matches the logic for `skeema push`.
+* Dropping a table
+* Altering a table to drop an existing column
+* Altering a table to change the type of an existing column in a way that potentially causes data loss, length truncation, or reduction in precision
+* Altering a table to change the character set of an existing column
+* Altering a table to change its storage engine
-You may also configure Skeema to always permit dropping tables or columns below a certain size (in bytes), or always permit dropping tables or columns only for tables that have no rows. See the [allow-below-size option](options.md#allow-below-size).
+Note that `skeema diff` also provides the [allow-unsafe option](options.md#allow-unsafe), even though `skeema diff` never actually modifies tables regardless. This option is present so that `skeema diff` can serve as a safe dry-run that exactly matches the logic for `skeema push`. If not explicitly allowed, `skeema diff` will display unsafe operations as commented-out DDL.
+
+You may also configure Skeema to always permit unsafe operations on tables below a certain size (in bytes), or always permit unsafe operations on tables that have no rows. See the [safe-below-size option](options.md#safe-below-size).
#### Auto-generated DDL is verified for correctness
View
@@ -3,9 +3,7 @@
### Index
* [all](#all)
-* [allow-below-size](#allow-below-size)
-* [allow-drop-column](#allow-drop-column)
-* [allow-drop-table](#allow-drop-table)
+* [allow-unsafe](#allow-unsafe)
* [alter-algorithm](#alter-algorithm)
* [alter-lock](#alter-lock)
* [alter-wrapper](#alter-wrapper)
@@ -25,6 +23,7 @@
* [password](#password)
* [port](#port)
* [reuse-temp-schema](#reuse-temp-schema)
+* [safe-below-size](#safe-below-size)
* [schema](#schema)
* [socket](#socket)
* [temp-schema](#temp-schema)
@@ -45,49 +44,27 @@ Ordinarily, for individual directories that map to multiple instances and/or mul
This is one of the only cases where the options for `skeema diff` differ from `skeema push`. The behavior in this respect is the opposite for `skeema push`: that command defaults to operating on all instances and schemas, unless the [first-only](#first-only) option is used, in which case it only operates on the first instance and schema per directory.
-### allow-below-size
-
-Commands | diff, push
---- | :---
-**Default** | 0
-**Type** | size
-**Restrictions** | none
-
-For any table below the specified size (in bytes), Skeema will allow execution of DROP TABLE statements, even if [allow-drop-table](#allow-drop-table) has not be enabled; and it will also allow ALTER TABLE ... DROP COLUMN statements, even if [allow-drop-column](#allow-drop-column) has not be enabled.
-
-The size comparison is a strict less-than. This means that with the default value of 0, no drops will be allowed automatically, as no table can be less than 0 bytes.
-
-To only allow drops on *empty* tables (ones without any rows), set [allow-below-size](#allow-below-size) to 1. Skeema always treats empty tables as size 0 bytes as a special-case.
-
-This option is intended to permit rapid development when altering a new table before it's in use, or dropping a table that was never in use. The intended pattern is to set [allow-below-size](#allow-below-size) in a global option file, likely to a higher value in the development environment and a lower value in the production environment. Then, whenever drops of a larger size are needed, the user should supply [--allow-drop-table](#allow-drop-table) or [--allow-drop-column](#allow-drop-column) *manually on the command-line* when appropriate.
-
-### allow-drop-column
+### allow-unsafe
Commands | diff, push
--- | :---
**Default** | false
**Type** | boolean
**Restrictions** | none
-If set to false, `skeema diff` outputs ALTER TABLE statements containing at least one DROP COLUMN clause as commented-out, and `skeema push` skips their execution. Note that the entire ALTER TABLE statement is skipped in this case, even if it contained additional clauses besides the DROP COLUMN clause. (This is to prevent problems with column renames, which Skeema does not yet support.)
-
-If set to true, ALTER TABLE ... DROP COLUMN statements are always permitted, regardless of table size and regardless of use of the [allow-below-size](#allow-below-size) option.
+If set to false, `skeema diff` outputs unsafe DDL statements as commented-out, and `skeema push` skips their execution.
-It is not recommended to enable this setting in an option file, especially in the production environment. It is safer to require users to supply it manually on the command-line on an as-needed basis.
-
-### allow-drop-table
-
-Commands | diff, push
---- | :---
-**Default** | false
-**Type** | boolean
-**Restrictions** | none
+The following operations are considered unsafe:
-If set to false, `skeema diff` outputs DROP TABLE statements as commented-out, and `skeema push` skips their execution.
+* Any DROP TABLE statement
+* Any ALTER TABLE statement that includes at least one DROP COLUMN clause
+* Any ALTER TABLE statement that includes a MODIFY COLUMN clause which changes the type of an existing column in a way that potentially causes data loss, length truncation, or reduction in precision
+* Any ALTER TABLE statement that includes a MODIFY COLUMN clause which changes the character set of an existing column
+* Any ALTER TABLE statement that includes an ENGINE clause which changes the table's storage engine
-If set to true, DROP TABLE statements are always permitted, regardless of table size and regardless of use of the [allow-below-size](#allow-below-size) option.
+If set to true, these operations are fully permitted, for all tables. It is not recommended to enable this setting in an option file, especially in the production environment. It is safer to require users to supply it manually on the command-line on an as-needed basis, to serve as a confirmation step for unsafe operations.
-It is not recommended to enable this setting in an option file, especially in the production environment. It is safer to require users to supply it manually on the command-line on an as-needed basis.
+To conditionally control execution of unsafe operations based on table size, see the [safe-below-size](#safe-below-size) option.
### alter-algorithm
@@ -261,7 +238,7 @@ This option enables debug logging in all commands. The extra output is sent to S
* When `skeema diff` or `skeema push` encounters tables that cannot be ALTERed due to use of features not yet supported by Skeema, the debug log will indicate which specific line(s) of the CREATE TABLE statement are using such features.
* When any command encounters non-fatal problems in a *.sql file, they will be logged. This can include extra ignored statements before/after the CREATE TABLE statement, or a table whose name does not match its filename.
* If a panic occurs in Skeema's main thread, a full stack trace will be logged.
-* Options that control conditional logic based on table sizes, such as [allow-below-size](#allow-below-size) and [alter-wrapper-min-size](#alter-wrapper-min-size), provide debug output with size information whenever their condition is triggered.
+* Options that control conditional logic based on table sizes, such as [safe-below-size](#safe-below-size) and [alter-wrapper-min-size](#alter-wrapper-min-size), provide debug output with size information whenever their condition is triggered.
* Upon exiting, the numeric exit code will be logged.
### default-character-set
@@ -428,6 +405,22 @@ If false, each Skeema operation will create a temporary schema, perform some DDL
This option most likely does not impact the list of privileges required for Skeema's user, since CREATE and DROP privileges will still be needed on the temporary schema to create or drop tables within the schema.
+### safe-below-size
+
+Commands | diff, push
+--- | :---
+**Default** | 0
+**Type** | size
+**Restrictions** | none
+
+For any table below the specified size (in bytes), Skeema will allow execution of unsafe operations, even if [allow-unsafe](#allow-unsafe) has not be enabled. (To see a list of which operations are considered unsafe, see the documentation for [allow-unsafe](#allow-unsafe).)
+
+The size comparison is a strict less-than. This means that with the default value of 0, no unsafe operations will be allowed automatically, as no table can be less than 0 bytes.
+
+To only allow unsafe operations on *empty* tables (ones without any rows), set [safe-below-size](#safe-below-size) to 1. Skeema always treats empty tables as size 0 bytes as a special-case.
+
+This option is intended to permit rapid development when altering a new table before it's in use, or dropping a table that was never in use. The intended pattern is to set [safe-below-size](#safe-below-size) in a global option file, potentially to a higher value in the development environment and a lower value in the production environment. This way, whenever unsafe operations are to be run on a larger table, the user must supply [--allow-unsafe](#allow-unsafe) *manually on the command-line* when appropriate to confirm the action.
+
### schema
Commands | *all*
Oops, something went wrong.

0 comments on commit b1d438c

Please sign in to comment.