Merge pull request #14256 from greenpeace-cee/innodb-log-tables

dev/core#663 - Use InnoDB engine for extended log tables

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,31 @@ 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?
+   *     '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;
-
+  public function updateLogTableSchema($params) {
     $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 +778,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 +787,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);
 

api/v3/System.php

@@ -413,6 +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,
   ];
 }
 

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();
   }
 

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(['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(['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);
+    }
+  }
+
+  public function alterLogTables(&$logTableSpec) {
+    foreach (array_keys($logTableSpec) as $tableName) {
+      $logTableSpec[$tableName]['engine'] = 'MyISAM';
+    }
   }
 
   /**
@@ -139,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']);