From fb8da6f4afe5f4a79977a1f7b0d9ce09b86086d7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 18 Sep 2014 08:32:21 -0700 Subject: [PATCH] Support key schemata and column nullability Summary: Ref T1191. The major issue motivation here is that InnoDB keys have a maximum length of 767 bytes. When we move `utf8` colums to `utf8mb4` columns, they'll jump from 3 bytes per character to 4 bytes per character, which may make some indexes too long. Add key schema to help spot this. Also add nullability since it doesn't hurt. Test Plan: See screenshots. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T1191 Differential Revision: https://secure.phabricator.com/D10499 --- src/__phutil_library_map__.php | 2 + .../PhabricatorConfigApplication.php | 2 +- .../PhabricatorConfigDatabaseController.php | 177 +++++++++++++++++- .../schema/PhabricatorConfigColumnSchema.php | 24 +++ .../schema/PhabricatorConfigKeySchema.php | 37 ++++ .../schema/PhabricatorConfigSchemaQuery.php | 79 +++++++- .../schema/PhabricatorConfigStorageSchema.php | 11 ++ .../schema/PhabricatorConfigTableSchema.php | 22 ++- 8 files changed, 338 insertions(+), 16 deletions(-) create mode 100644 src/applications/config/schema/PhabricatorConfigKeySchema.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2e036f89f9..de93cc321c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1349,6 +1349,7 @@ phutil_register_library_map(array( 'PhabricatorConfigIssueViewController' => 'applications/config/controller/PhabricatorConfigIssueViewController.php', 'PhabricatorConfigJSON' => 'applications/config/json/PhabricatorConfigJSON.php', 'PhabricatorConfigJSONOptionType' => 'applications/config/custom/PhabricatorConfigJSONOptionType.php', + 'PhabricatorConfigKeySchema' => 'applications/config/schema/PhabricatorConfigKeySchema.php', 'PhabricatorConfigListController' => 'applications/config/controller/PhabricatorConfigListController.php', 'PhabricatorConfigLocalSource' => 'infrastructure/env/PhabricatorConfigLocalSource.php', 'PhabricatorConfigManagementDeleteWorkflow' => 'applications/config/management/PhabricatorConfigManagementDeleteWorkflow.php', @@ -4242,6 +4243,7 @@ phutil_register_library_map(array( 'PhabricatorConfigIssueListController' => 'PhabricatorConfigController', 'PhabricatorConfigIssueViewController' => 'PhabricatorConfigController', 'PhabricatorConfigJSONOptionType' => 'PhabricatorConfigOptionType', + 'PhabricatorConfigKeySchema' => 'PhabricatorConfigStorageSchema', 'PhabricatorConfigListController' => 'PhabricatorConfigController', 'PhabricatorConfigLocalSource' => 'PhabricatorConfigProxySource', 'PhabricatorConfigManagementDeleteWorkflow' => 'PhabricatorConfigManagementWorkflow', diff --git a/src/applications/config/application/PhabricatorConfigApplication.php b/src/applications/config/application/PhabricatorConfigApplication.php index 47cc960f71..59f8f8e959 100644 --- a/src/applications/config/application/PhabricatorConfigApplication.php +++ b/src/applications/config/application/PhabricatorConfigApplication.php @@ -45,7 +45,7 @@ final class PhabricatorConfigApplication extends PhabricatorApplication { 'database/'. '(?:(?P[^/]+)/'. '(?:(?P[^/]+)/'. - '(?:(?P[^/]+)/)?)?)?' + '(?:(?:col/(?P[^/]+)|key/(?P[^/]+))/)?)?)?' => 'PhabricatorConfigDatabaseController', '(?Pignore|unignore)/(?P[^/]+)/' => 'PhabricatorConfigIgnoreController', diff --git a/src/applications/config/controller/PhabricatorConfigDatabaseController.php b/src/applications/config/controller/PhabricatorConfigDatabaseController.php index 4cd02ae126..20f89b0162 100644 --- a/src/applications/config/controller/PhabricatorConfigDatabaseController.php +++ b/src/applications/config/controller/PhabricatorConfigDatabaseController.php @@ -3,14 +3,18 @@ final class PhabricatorConfigDatabaseController extends PhabricatorConfigController { + const MAX_INNODB_KEY_LENGTH = 767; + private $database; private $table; private $column; + private $key; public function willProcessRequest(array $data) { $this->database = idx($data, 'database'); $this->table = idx($data, 'table'); $this->column = idx($data, 'column'); + $this->key = idx($data, 'key'); } public function processRequest() { @@ -43,6 +47,14 @@ final class PhabricatorConfigDatabaseController $this->database, $this->table, $this->column); + } else if ($this->key) { + return $this->renderKey( + $comp, + $expect, + $actual, + $this->database, + $this->table, + $this->key); } else if ($this->table) { return $this->renderTable( $comp, @@ -77,12 +89,16 @@ final class PhabricatorConfigDatabaseController $crumbs->addTextCrumb( $this->database, $this->getApplicationURI('database/'.$this->database.'/')); - if ($this->column) { + if ($this->column || $this->key) { $crumbs->addTextCrumb( $this->table, $this->getApplicationURI( 'database/'.$this->database.'/'.$this->table.'/')); - $crumbs->addTextCrumb($this->column); + if ($this->column) { + $crumbs->addTextCrumb($this->column); + } else { + $crumbs->addTextCrumb($this->key); + } } else { $crumbs->addTextCrumb($this->table); } @@ -276,6 +292,7 @@ final class PhabricatorConfigDatabaseController $type_issue = PhabricatorConfigStorageSchema::ISSUE_COLUMNTYPE; $charset_issue = PhabricatorConfigStorageSchema::ISSUE_CHARSET; $collation_issue = PhabricatorConfigStorageSchema::ISSUE_COLLATION; + $nullable_issue = PhabricatorConfigStorageSchema::ISSUE_NULLABLE; $database = $comp->getDatabase($database_name); if (!$database) { @@ -322,6 +339,7 @@ final class PhabricatorConfigDatabaseController 'database/'. $database_name.'/'. $table_name.'/'. + 'col/'. $column_name.'/'), ), $column_name), @@ -329,6 +347,11 @@ final class PhabricatorConfigDatabaseController $this->renderAttr( $column->getColumnType(), $column->hasIssue($type_issue)), + $this->renderAttr( + $column->getNullable() + ? pht('Yes') + : pht('No'), + $column->hasIssue($nullable_issue)), $this->renderAttr( $column->getCharacterSet(), $column->hasIssue($charset_issue)), @@ -342,9 +365,10 @@ final class PhabricatorConfigDatabaseController ->setHeaders( array( null, - pht('Table'), + pht('Column'), pht('Data Type'), pht('Column Type'), + pht('Nullable'), pht('Character Set'), pht('Collation'), )) @@ -358,6 +382,66 @@ final class PhabricatorConfigDatabaseController null )); + $key_rows = array(); + foreach ($table->getKeys() as $key_name => $key) { + $expect_key = null; + if ($expect_table) { + $expect_key = $expect_table->getKey($key_name); + } + + $status = $key->getStatus(); + + $size = 0; + foreach ($key->getColumnNames() as $column_name) { + $column = $table->getColumn($column_name); + if (!$column) { + $size = 0; + break; + } + $size += $column->getKeyByteLength(); + } + + $size_formatted = null; + if ($size) { + $size_formatted = $this->renderAttr( + $size, + ($size > self::MAX_INNODB_KEY_LENGTH)); + } + + $key_rows[] = array( + $this->renderIcon($status), + phutil_tag( + 'a', + array( + 'href' => $this->getApplicationURI( + 'database/'. + $database_name.'/'. + $table_name.'/'. + 'key/'. + $key_name.'/'), + ), + $key_name), + implode(', ', $key->getColumnNames()), + $size_formatted, + ); + } + + $keys_view = id(new AphrontTableView($key_rows)) + ->setHeaders( + array( + null, + pht('Key'), + pht('Columns'), + pht('Size'), + )) + ->setColumnClasses( + array( + null, + 'wide pri', + null, + null, + )); + $title = pht('Database Status: %s.%s', $database_name, $table_name); if ($actual_table) { @@ -388,7 +472,8 @@ final class PhabricatorConfigDatabaseController $box = id(new PHUIObjectBoxView()) ->setHeaderText($title) ->addPropertyList($properties) - ->appendChild($table_view); + ->appendChild($table_view) + ->appendChild($keys_view); return $this->buildResponse($title, $box); } @@ -412,7 +497,7 @@ final class PhabricatorConfigDatabaseController } $column = $table->getColumn($column_name); - if (!$table) { + if (!$column) { return new Aphront404Response(); } @@ -504,6 +589,88 @@ final class PhabricatorConfigDatabaseController return $this->buildResponse($title, $box); } + + private function renderKey( + PhabricatorConfigServerSchema $comp, + PhabricatorConfigServerSchema $expect, + PhabricatorConfigServerSchema $actual, + $database_name, + $table_name, + $key_name) { + + $database = $comp->getDatabase($database_name); + if (!$database) { + return new Aphront404Response(); + } + + $table = $database->getTable($table_name); + if (!$table) { + return new Aphront404Response(); + } + + $key = $table->getKey($key_name); + if (!$key) { + return new Aphront404Response(); + } + + $actual_database = $actual->getDatabase($database_name); + $actual_table = null; + $actual_key = null; + if ($actual_database) { + $actual_table = $actual_database->getTable($table_name); + if ($actual_table) { + $actual_key = $actual_table->getKey($key_name); + } + } + + $expect_database = $expect->getDatabase($database_name); + $expect_table = null; + $expect_key = null; + if ($expect_database) { + $expect_table = $expect_database->getTable($table_name); + if ($expect_table) { + $expect_key = $expect_table->getKey($key_name); + } + } + + if ($actual_key) { + $actual_columns = $actual_key->getColumnNames(); + } else { + $actual_columns = array(); + } + + if ($expect_key) { + $expect_columns = $expect_key->getColumnNames(); + } else { + $expect_columns = array(); + } + + $title = pht( + 'Database Status: %s.%s (%s)', + $database_name, + $table_name, + $key_name); + + $properties = $this->buildProperties( + array( + array( + pht('Columns'), + implode(', ', $actual_columns), + ), + array( + pht('Expected Columns'), + implode(', ', $expect_columns), + ), + ), + $key->getIssues()); + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText($title) + ->addPropertyList($properties); + + return $this->buildResponse($title, $box); + } + private function renderIcon($status) { switch ($status) { case PhabricatorConfigStorageSchema::STATUS_OKAY: diff --git a/src/applications/config/schema/PhabricatorConfigColumnSchema.php b/src/applications/config/schema/PhabricatorConfigColumnSchema.php index 26fc58070d..3a33359352 100644 --- a/src/applications/config/schema/PhabricatorConfigColumnSchema.php +++ b/src/applications/config/schema/PhabricatorConfigColumnSchema.php @@ -7,6 +7,16 @@ final class PhabricatorConfigColumnSchema private $collation; private $columnType; private $dataType; + private $nullable; + + public function setNullable($nullable) { + $this->nullable = $nullable; + return $this; + } + + public function getNullable() { + return $this->nullable; + } public function setColumnType($column_type) { $this->columnType = $column_type; @@ -48,6 +58,20 @@ final class PhabricatorConfigColumnSchema return $this->characterSet; } + public function getKeyByteLength() { + $type = $this->getColumnType(); + + $matches = null; + if (preg_match('/^varchar\((\d+)\)$/', $type, $matches)) { + // For utf8mb4, each character requires 4 bytes. + return ((int)$matches[1]) * 4; + } + + // TODO: Build this out to catch overlong indexes. + + return 0; + } + public function compareToSimilarSchema( PhabricatorConfigStorageSchema $expect) { diff --git a/src/applications/config/schema/PhabricatorConfigKeySchema.php b/src/applications/config/schema/PhabricatorConfigKeySchema.php new file mode 100644 index 0000000000..1599c9a82e --- /dev/null +++ b/src/applications/config/schema/PhabricatorConfigKeySchema.php @@ -0,0 +1,37 @@ +columnNames = $column_names; + return $this; + } + + public function getColumnNames() { + return $this->columnNames; + } + + protected function getSubschemata() { + return array(); + } + + public function compareToSimilarSchema( + PhabricatorConfigStorageSchema $expect) { + + $issues = array(); + if ($this->getColumnNames() !== $expect->getColumnNames()) { + $issues[] = self::ISSUE_KEYCOLUMNS; + } + + return $issues; + } + + public function newEmptyClone() { + $clone = clone $this; + return $clone; + } + +} diff --git a/src/applications/config/schema/PhabricatorConfigSchemaQuery.php b/src/applications/config/schema/PhabricatorConfigSchemaQuery.php index 2e193dc120..837fb7d7b4 100644 --- a/src/applications/config/schema/PhabricatorConfigSchemaQuery.php +++ b/src/applications/config/schema/PhabricatorConfigSchemaQuery.php @@ -47,6 +47,41 @@ final class PhabricatorConfigSchemaQuery extends Phobject { $databases); $database_info = ipull($database_info, null, 'SCHEMA_NAME'); + $sql = array(); + foreach ($tables as $table) { + $sql[] = qsprintf( + $conn, + '(TABLE_SCHEMA = %s AND TABLE_NAME = %s)', + $table['TABLE_SCHEMA'], + $table['TABLE_NAME']); + } + + if ($sql) { + $column_info = queryfx_all( + $conn, + 'SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, CHARACTER_SET_NAME, + COLLATION_NAME, COLUMN_TYPE, IS_NULLABLE + FROM INFORMATION_SCHEMA.COLUMNS + WHERE (%Q)', + '('.implode(') OR (', $sql).')'); + $column_info = igroup($column_info, 'TABLE_SCHEMA'); + } else { + $column_info = array(); + } + + if ($sql) { + $key_info = queryfx_all( + $conn, + 'SELECT CONSTRAINT_NAME, TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, + ORDINAL_POSITION, POSITION_IN_UNIQUE_CONSTRAINT + FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE + WHERE (%Q)', + '('.implode(') OR (', $sql).')'); + $key_info = igroup($key_info, 'TABLE_SCHEMA'); + } else { + $key_info = array(); + } + $server_schema = new PhabricatorConfigServerSchema(); $tables = igroup($tables, 'TABLE_SCHEMA'); @@ -58,6 +93,11 @@ final class PhabricatorConfigSchemaQuery extends Phobject { ->setCharacterSet($info['DEFAULT_CHARACTER_SET_NAME']) ->setCollation($info['DEFAULT_COLLATION_NAME']); + $database_column_info = idx($column_info, $database_name, array()); + $database_column_info = igroup($database_column_info, 'TABLE_NAME'); + $database_key_info = idx($key_info, $database_name, array()); + $database_key_info = igroup($database_key_info, 'TABLE_NAME'); + foreach ($database_tables as $table) { $table_name = $table['TABLE_NAME']; @@ -65,24 +105,29 @@ final class PhabricatorConfigSchemaQuery extends Phobject { ->setName($table_name) ->setCollation($table['TABLE_COLLATION']); - $columns = queryfx_all( - $conn, - 'SELECT COLUMN_NAME, CHARACTER_SET_NAME, COLLATION_NAME, COLUMN_TYPE - FROM INFORMATION_SCHEMA.COLUMNS - WHERE TABLE_SCHEMA = %s AND TABLE_NAME = %s', - $database_name, - $table_name); - + $columns = idx($database_column_info, $table_name, array()); foreach ($columns as $column) { $column_schema = id(new PhabricatorConfigColumnSchema()) ->setName($column['COLUMN_NAME']) ->setCharacterSet($column['CHARACTER_SET_NAME']) ->setCollation($column['COLLATION_NAME']) - ->setColumnType($column['COLUMN_TYPE']); + ->setColumnType($column['COLUMN_TYPE']) + ->setNullable($column['IS_NULLABLE'] == 'YES'); $table_schema->addColumn($column_schema); } + $key_parts = idx($database_key_info, $table_name, array()); + $keys = igroup($key_parts, 'CONSTRAINT_NAME'); + foreach ($keys as $key_name => $key_pieces) { + $key_pieces = isort($key_pieces, 'ORDINAL_POSITION'); + $key_schema = id(new PhabricatorConfigKeySchema()) + ->setName($key_name) + ->setColumnNames(ipull($key_pieces, 'COLUMN_NAME')); + + $table_schema->addKey($key_schema); + } + $database_schema->addTable($table_schema); } @@ -190,6 +235,22 @@ final class PhabricatorConfigSchemaQuery extends Phobject { $comp_table->addColumn($comp_column); } + + $all_keys = + $actual_table->getKeys() + + $expect_table->getKeys(); + foreach ($all_keys as $key_name => $key_template) { + $actual_key = $actual_table->getKey($key_name); + $expect_key = $expect_table->getKey($key_name); + + $issues = $this->compareSchemata($expect_key, $actual_key); + + $comp_key = $key_template->newEmptyClone() + ->setIssues($issues); + + $comp_table->addKey($comp_key); + } + $comp_database->addTable($comp_table); } $comp_server->addDatabase($comp_database); diff --git a/src/applications/config/schema/PhabricatorConfigStorageSchema.php b/src/applications/config/schema/PhabricatorConfigStorageSchema.php index 2a04e79586..f32e51632e 100644 --- a/src/applications/config/schema/PhabricatorConfigStorageSchema.php +++ b/src/applications/config/schema/PhabricatorConfigStorageSchema.php @@ -7,6 +7,8 @@ abstract class PhabricatorConfigStorageSchema extends Phobject { const ISSUE_CHARSET = 'charset'; const ISSUE_COLLATION = 'collation'; const ISSUE_COLUMNTYPE = 'columntype'; + const ISSUE_NULLABLE = 'nullable'; + const ISSUE_KEYCOLUMNS = 'keycolumns'; const ISSUE_SUBWARN = 'subwarn'; const ISSUE_SUBFAIL = 'subfail'; @@ -98,6 +100,10 @@ abstract class PhabricatorConfigStorageSchema extends Phobject { return pht('Wrong Collation'); case self::ISSUE_COLUMNTYPE: return pht('Wrong Column Type'); + case self::ISSUE_NULLABLE: + return pht('Wrong Nullable Setting'); + case self::ISSUE_KEYCOLUMNS: + return pht('Key on Wrong Columns'); case self::ISSUE_SUBWARN: return pht('Subschemata Have Warnings'); case self::ISSUE_SUBFAIL: @@ -119,6 +125,10 @@ abstract class PhabricatorConfigStorageSchema extends Phobject { return pht('This schema can use a better collation.'); case self::ISSUE_COLUMNTYPE: return pht('This schema can use a better column type.'); + case self::ISSUE_NULLABLE: + return pht('This schema has the wrong nullable setting.'); + case self::ISSUE_KEYCOLUMNS: + return pht('This schema is on the wrong columns.'); case self::ISSUE_SUBWARN: return pht('Subschemata have setup warnings.'); case self::ISSUE_SUBFAIL: @@ -138,6 +148,7 @@ abstract class PhabricatorConfigStorageSchema extends Phobject { case self::ISSUE_COLLATION: case self::ISSUE_COLUMNTYPE: case self::ISSUE_SUBWARN: + case self::ISSUE_KEYCOLUMNS: return self::STATUS_WARN; default: throw new Exception(pht('Unknown schema issue "%s"!', $issue)); diff --git a/src/applications/config/schema/PhabricatorConfigTableSchema.php b/src/applications/config/schema/PhabricatorConfigTableSchema.php index 393afac794..81c23d68c3 100644 --- a/src/applications/config/schema/PhabricatorConfigTableSchema.php +++ b/src/applications/config/schema/PhabricatorConfigTableSchema.php @@ -5,6 +5,7 @@ final class PhabricatorConfigTableSchema private $collation; private $columns = array(); + private $keys = array(); public function addColumn(PhabricatorConfigColumnSchema $column) { $key = $column->getName(); @@ -16,6 +17,16 @@ final class PhabricatorConfigTableSchema return $this; } + public function addKey(PhabricatorConfigKeySchema $key) { + $name = $key->getName(); + if (isset($this->keys[$name])) { + throw new Exception( + pht('Trying to add duplicate key "%s"!', $name)); + } + $this->keys[$name] = $key; + return $this; + } + public function getColumns() { return $this->columns; } @@ -24,8 +35,16 @@ final class PhabricatorConfigTableSchema return idx($this->getColumns(), $key); } + public function getKeys() { + return $this->keys; + } + + public function getKey($key) { + return idx($this->getKeys(), $key); + } + protected function getSubschemata() { - return $this->getColumns(); + return array_merge($this->getColumns(), $this->getKeys()); } public function setCollation($collation) { @@ -51,6 +70,7 @@ final class PhabricatorConfigTableSchema public function newEmptyClone() { $clone = clone $this; $clone->columns = array(); + $clone->keys = array(); return $clone; }