From 204aa6fb2ab0e0a2c844440cc8014b5daed4164f Mon Sep 17 00:00:00 2001 From: Patrick Figel Date: Sat, 8 Jun 2019 12:19:15 +0200 Subject: [PATCH 1/2] dev/core#663 - Use InnoDB engine for extended log tables This changes the default storage engine used for log tables to InnoDB in order to provide safer defaults. Existing log tables are not upgraded unless explicitly requested using the new forceEngineMigration System.updatelogtables API parameter. Hook overrides are respected. --- CRM/Logging/Schema.php | 36 ++++++++++++---- api/v3/System.php | 5 +++ tests/phpunit/CRM/Logging/LoggingTest.php | 11 +++-- tests/phpunit/CRM/Logging/SchemaTest.php | 50 +++++++++++++++++++++++ 4 files changed, 91 insertions(+), 11 deletions(-) diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php index 9ce4883b1813..bc0eb23ec1cd 100644 --- a/CRM/Logging/Schema.php +++ b/CRM/Logging/Schema.php @@ -31,6 +31,14 @@ * @copyright CiviCRM LLC (c) 2004-2019 */ class CRM_Logging_Schema { + + /** + * Default storage engine for log tables + * + * @var string + */ + const ENGINE = 'InnoDB'; + private $logs = []; private $tables = []; @@ -58,7 +66,7 @@ class CRM_Logging_Schema { /** * Specifications of all log table including - * - engine (default is archive, if not set.) + * - engine (default is InnoDB, if not set.) * - engine_config, a string appended to the engine type. * For INNODB space can be saved with 'ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=4' * - indexes (default is none and they cannot be added unless engine is innodb. If they are added and @@ -302,23 +310,36 @@ public function fixSchemaDifferences($enableLogging = FALSE) { * and also implements the engine change defined by the hook (i.e. INNODB). * * Note changing engine & adding hook-defined indexes, but not changing back - * to ARCHIVE if engine has not been deliberately set (by hook) and not dropping - * indexes. Sysadmin will need to manually intervene to revert to defaults. + * to INNODB if engine has not been deliberately set (by hook) and not + * dropping indexes. Sysadmin will need to manually intervene to revert to + * defaults. * * @param array $params - * 'updateChangedEngineConfig' - update if the engine config changes, default FALSE + * 'updateChangedEngineConfig' - update if the engine config changes, + * default FALSE + * 'forceEngineMigration' - force engine upgrade from ARCHIVE to InnoDB, + * default FALSE * * @return int $updateTablesCount + * @throws \CiviCRM_API3_Exception */ public function updateLogTableSchema($params = []) { isset($params['updateChangedEngineConfig']) ? NULL : $params['updateChangedEngineConfig'] = FALSE; + isset($params['forceEngineMigration']) ? NULL : $params['forceEngineMigration'] = FALSE; $updateLogConn = FALSE; $updatedTablesCount = 0; foreach ($this->logs as $mainTable => $logTable) { $alterSql = []; $tableSpec = $this->logTableSpec[$mainTable]; - $engineChanged = isset($tableSpec['engine']) && (strtoupper($tableSpec['engine']) != $this->getEngineForLogTable($logTable)); + $currentEngine = strtoupper($this->getEngineForLogTable($logTable)); + if (!isset($tableSpec['engine']) && $currentEngine == 'ARCHIVE' && $params['forceEngineMigration']) { + // table uses ARCHIVE engine (the previous default) and no one set an + // alternative engine via hook_civicrm_alterLogTables => force change to + // new default + $tableSpec['engine'] = self::ENGINE; + } + $engineChanged = isset($tableSpec['engine']) && (strtoupper($tableSpec['engine']) != $currentEngine); $engineConfigChanged = isset($tableSpec['engine_config']) && (strtoupper($tableSpec['engine_config']) != $this->getEngineConfigForLogTable($logTable)); if ($engineChanged || ($engineConfigChanged && $params['updateChangedEngineConfig'])) { $alterSql[] = "ENGINE=" . $tableSpec['engine'] . " " . CRM_Utils_Array::value('engine_config', $tableSpec); @@ -762,7 +783,7 @@ private function createLogTableFor($table) { // - prepend the name with log_ // - drop AUTO_INCREMENT columns // - drop non-column rows of the query (keys, constraints, etc.) - // - set the ENGINE to the specified engine (default is archive or if archive is disabled or nor installed INNODB) + // - set the ENGINE to the specified engine (default is INNODB) // - add log-specific columns (at the end of the table) $mysqlEngines = []; $engines = CRM_Core_DAO::executeQuery("SHOW ENGINES"); @@ -771,11 +792,10 @@ private function createLogTableFor($table) { $mysqlEngines[] = $engines->Engine; } } - $logEngine = in_array('ARCHIVE', $mysqlEngines) ? 'ARCHIVE' : 'INNODB'; $query = preg_replace("/^CREATE TABLE `$table`/i", "CREATE TABLE `{$this->db}`.log_$table", $query); $query = preg_replace("/ AUTO_INCREMENT/i", '', $query); $query = preg_replace("/^ [^`].*$/m", '', $query); - $engine = strtoupper(CRM_Utils_Array::value('engine', $this->logTableSpec[$table], $logEngine)); + $engine = strtoupper(CRM_Utils_Array::value('engine', $this->logTableSpec[$table], self::ENGINE)); $engine .= " " . CRM_Utils_Array::value('engine_config', $this->logTableSpec[$table]); $query = preg_replace("/^\) ENGINE=[^ ]+ /im", ') ENGINE=' . $engine . ' ', $query); diff --git a/api/v3/System.php b/api/v3/System.php index 35238776820f..5d52ba734a58 100644 --- a/api/v3/System.php +++ b/api/v3/System.php @@ -414,6 +414,11 @@ function _civicrm_api3_system_updatelogtables_spec(&$params) { 'description' => 'By default, we only update if the ENGINE has changed, set this to TRUE to update if the ENGINE_CONFIG has changed.', 'type' => CRM_Utils_Type::T_BOOLEAN, ]; + $params['forceEngineMigration'] = [ + 'title' => 'Force storage engine to upgrade to InnoDB?', + 'description' => 'Older versions of CiviCRM used the ARCHIVE engine by default. Set this to TRUE to migrate the engine to the new default.', + 'type' => CRM_Utils_Type::T_BOOLEAN, + ]; } /** diff --git a/tests/phpunit/CRM/Logging/LoggingTest.php b/tests/phpunit/CRM/Logging/LoggingTest.php index 62d242320112..05ed23a17dcc 100644 --- a/tests/phpunit/CRM/Logging/LoggingTest.php +++ b/tests/phpunit/CRM/Logging/LoggingTest.php @@ -47,8 +47,11 @@ public function testMultilingualAlterSchemaLogging() { $query = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE `log_civicrm_option_value`", array(), TRUE, NULL, FALSE, FALSE); $query->fetch(); $create = explode("\n", $query->Create_Table); - $this->assertTrue(in_array(" `logging_test` int(11) DEFAULT '0'", $create)); - $create = explode("\n", $query->Create_Table); + // MySQL may return "DEFAULT 0" or "DEFAULT '0'" depending on version + $this->assertTrue( + in_array(" `logging_test` int(11) DEFAULT '0'", $create) + || in_array(" `logging_test` int(11) DEFAULT 0", $create) + ); CRM_Core_DAO::executeQuery("ALTER TABLE `civicrm_option_value` DROP COLUMN `logging_test`", array(), FALSE, NULL, FALSE, FALSE); $query = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE `log_civicrm_option_value`", array(), TRUE, NULL, FALSE, FALSE); $query->fetch(); @@ -58,7 +61,9 @@ public function testMultilingualAlterSchemaLogging() { \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = array(); CRM_Core_I18n_Schema::rebuildMultilingualSchema($locales); $logging->fixSchemaDifferencesFor('civicrm_option_value', array(), TRUE); - $this->assertTrue(in_array(" `logging_test` int(11) DEFAULT '0'", $create)); + $this->assertTrue( + in_array(" `logging_test` int(11) DEFAULT '0'", $create) + || in_array(" `logging_test` int(11) DEFAULT 0", $create)); $logging->disableLogging(); } diff --git a/tests/phpunit/CRM/Logging/SchemaTest.php b/tests/phpunit/CRM/Logging/SchemaTest.php index efdd8ccb2b5c..f49c76d35087 100644 --- a/tests/phpunit/CRM/Logging/SchemaTest.php +++ b/tests/phpunit/CRM/Logging/SchemaTest.php @@ -37,13 +37,63 @@ public function testQueryRewrite($query, $expectedQuery) { $this->assertEquals($expectedQuery, CRM_Logging_Schema::fixTimeStampAndNotNullSQL($query)); } + /** + * Test log tables are created as InnoDB by default + */ public function testLogEngine() { $schema = new CRM_Logging_Schema(); $schema->enableLogging(); $log_table = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE log_civicrm_acl"); + while ($log_table->fetch()) { + $this->assertRegexp('/ENGINE=InnoDB/', $log_table->Create_Table); + } + } + + /** + * Test that the log table engine can be changed via hook to e.g. MyISAM + */ + public function testHookLogEngine() { + $this->hookClass->setHook('civicrm_alterLogTables', array($this, 'alterLogTables')); + $schema = new CRM_Logging_Schema(); + $schema->enableLogging(); + $log_table = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE log_civicrm_acl"); + while ($log_table->fetch()) { + $this->assertRegexp('/ENGINE=MyISAM/', $log_table->Create_Table); + } + } + + /** + * Test that existing log tables with ARCHIVE engine are converted to InnoDB + * + * @throws \Exception + */ + public function testArchiveEngineConversion() { + $schema = new CRM_Logging_Schema(); + $schema->enableLogging(); + // change table to ARCHIVE + CRM_Core_DAO::executeQuery("ALTER TABLE log_civicrm_acl ENGINE ARCHIVE"); + $log_table = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE log_civicrm_acl"); + while ($log_table->fetch()) { + $this->assertRegexp('/ENGINE=ARCHIVE/', $log_table->Create_Table); + } + // engine should not change by default + $schema->updateLogTableSchema(); + $log_table = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE log_civicrm_acl"); while ($log_table->fetch()) { $this->assertRegexp('/ENGINE=ARCHIVE/', $log_table->Create_Table); } + // update with forceEngineMigration should convert to InnoDB + $schema->updateLogTableSchema(['forceEngineMigration' => TRUE]); + $log_table = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE log_civicrm_acl"); + while ($log_table->fetch()) { + $this->assertRegexp('/ENGINE=InnoDB/', $log_table->Create_Table); + } + } + + public function alterLogTables(&$logTableSpec) { + foreach (array_keys($logTableSpec) as $tableName) { + $logTableSpec[$tableName]['engine'] = 'MyISAM'; + } } /** From e26df891420908fff6d5112a15957d5779b4a124 Mon Sep 17 00:00:00 2001 From: Patrick Figel Date: Fri, 14 Jun 2019 20:48:23 +0200 Subject: [PATCH 2/2] Add API defaults in System.updatelogtables This adds API defaults for the updateChangedEngineConfig and forceEngineMigration parameters of System.updatelogtables and removes some fallback code that's unnecessary now. --- CRM/Logging/Schema.php | 11 +++-------- api/v3/System.php | 2 ++ tests/phpunit/CRM/Logging/SchemaTest.php | 6 +++--- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php index bc0eb23ec1cd..efbeb831c7b4 100644 --- a/CRM/Logging/Schema.php +++ b/CRM/Logging/Schema.php @@ -315,18 +315,13 @@ public function fixSchemaDifferences($enableLogging = FALSE) { * defaults. * * @param array $params - * 'updateChangedEngineConfig' - update if the engine config changes, - * default FALSE - * 'forceEngineMigration' - force engine upgrade from ARCHIVE to InnoDB, - * default FALSE + * 'updateChangedEngineConfig' - update if the engine config changes? + * 'forceEngineMigration' - force engine upgrade from ARCHIVE to InnoDB? * * @return int $updateTablesCount * @throws \CiviCRM_API3_Exception */ - public function updateLogTableSchema($params = []) { - isset($params['updateChangedEngineConfig']) ? NULL : $params['updateChangedEngineConfig'] = FALSE; - isset($params['forceEngineMigration']) ? NULL : $params['forceEngineMigration'] = FALSE; - + public function updateLogTableSchema($params) { $updateLogConn = FALSE; $updatedTablesCount = 0; foreach ($this->logs as $mainTable => $logTable) { diff --git a/api/v3/System.php b/api/v3/System.php index 5d52ba734a58..54faf069a1a3 100644 --- a/api/v3/System.php +++ b/api/v3/System.php @@ -413,11 +413,13 @@ function _civicrm_api3_system_updatelogtables_spec(&$params) { 'title' => 'Update Engine Config if changed?', 'description' => 'By default, we only update if the ENGINE has changed, set this to TRUE to update if the ENGINE_CONFIG has changed.', 'type' => CRM_Utils_Type::T_BOOLEAN, + 'api.default' => FALSE, ]; $params['forceEngineMigration'] = [ 'title' => 'Force storage engine to upgrade to InnoDB?', 'description' => 'Older versions of CiviCRM used the ARCHIVE engine by default. Set this to TRUE to migrate the engine to the new default.', 'type' => CRM_Utils_Type::T_BOOLEAN, + 'api.default' => FALSE, ]; } diff --git a/tests/phpunit/CRM/Logging/SchemaTest.php b/tests/phpunit/CRM/Logging/SchemaTest.php index f49c76d35087..f7fcdb8e51ae 100644 --- a/tests/phpunit/CRM/Logging/SchemaTest.php +++ b/tests/phpunit/CRM/Logging/SchemaTest.php @@ -77,13 +77,13 @@ public function testArchiveEngineConversion() { $this->assertRegexp('/ENGINE=ARCHIVE/', $log_table->Create_Table); } // engine should not change by default - $schema->updateLogTableSchema(); + $schema->updateLogTableSchema(['updateChangedEngineConfig' => FALSE, 'forceEngineMigration' => FALSE]); $log_table = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE log_civicrm_acl"); while ($log_table->fetch()) { $this->assertRegexp('/ENGINE=ARCHIVE/', $log_table->Create_Table); } // update with forceEngineMigration should convert to InnoDB - $schema->updateLogTableSchema(['forceEngineMigration' => TRUE]); + $schema->updateLogTableSchema(['updateChangedEngineConfig' => FALSE, 'forceEngineMigration' => TRUE]); $log_table = CRM_Core_DAO::executeQuery("SHOW CREATE TABLE log_civicrm_acl"); while ($log_table->fetch()) { $this->assertRegexp('/ENGINE=InnoDB/', $log_table->Create_Table); @@ -189,7 +189,7 @@ public function testColumnInfo() { ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci"); $schema = new CRM_Logging_Schema(); $schema->enableLogging(); - $schema->updateLogTableSchema(); + $schema->updateLogTableSchema(['updateChangedEngineConfig' => FALSE, 'forceEngineMigration' => FALSE]); $ci = \Civi::$statics['CRM_Logging_Schema']['columnSpecs']['civicrm_test_column_info']; $this->assertEquals('test_id', $ci['test_id']['COLUMN_NAME']);