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

civicrm · Jun 8, 2019 · 7b56be8 · 7b56be8
1 parent 5976ea1
commit 7b56be8
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 11 deletions.
diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php
@@ -31,6 +31,12 @@
  * @copyright CiviCRM LLC (c) 2004-2019
  */
 class CRM_Logging_Schema {
+
+  /**
+   * Default storage engine for log tables
+   */
+  const ENGINE = 'InnoDB';
+
   private $logs = [];
   private $tables = [];
 
@@ -58,7 +64,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 +308,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 +781,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 +790,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
@@ -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
@@ -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
@@ -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';
+    }
   }
 
   /**