From 3f17d7091b05eb1d433b43aa6e86459f949d1e25 Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Mon, 20 Jan 2014 14:06:44 +0800 Subject: [PATCH 1/3] MDL-43761 refactor db manager code to send arrays of SQL to the dml driver --- lib/ddl/database_manager.php | 15 ++++------- lib/dml/moodle_database.php | 4 +-- lib/dml/mssql_native_moodle_database.php | 23 +++++++++++----- lib/dml/mysqli_native_moodle_database.php | 23 +++++++++++----- lib/dml/oci_native_moodle_database.php | 27 ++++++++++++------- lib/dml/pdo_moodle_database.php | 32 ++++++++++++++++------- lib/dml/pgsql_native_moodle_database.php | 25 ++++++++++++------ lib/dml/sqlsrv_native_moodle_database.php | 23 +++++++++++----- 8 files changed, 112 insertions(+), 60 deletions(-) diff --git a/lib/ddl/database_manager.php b/lib/ddl/database_manager.php index 1112bafb203..741f7904363 100644 --- a/lib/ddl/database_manager.php +++ b/lib/ddl/database_manager.php @@ -69,26 +69,21 @@ class database_manager { /** * This function will execute an array of SQL commands. * - * @param array $sqlarr Array of sql statements to execute. - * @throws ddl_exception This exception is thrown if any error is found. + * @param string[] $sqlarr Array of sql statements to execute. + * @throws ddl_change_structure_exception This exception is thrown if any error is found. */ protected function execute_sql_arr(array $sqlarr) { - foreach ($sqlarr as $sql) { - $this->execute_sql($sql); - } + $this->mdb->change_database_structure($sqlarr); } /** * Execute a given sql command string. * * @param string $sql The sql string you wish to be executed. - * @throws ddl_exception This exception is thrown if any error is found. + * @throws ddl_change_structure_exception This exception is thrown if any error is found. */ protected function execute_sql($sql) { - if (!$this->mdb->change_database_structure($sql)) { - // in case driver does not throw exceptions yet ;-) - throw new ddl_change_structure_exception($this->mdb->get_last_error(), $sql); - } + $this->mdb->change_database_structure($sql); } /** diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php index 070f0cb9eff..34673255ea7 100644 --- a/lib/dml/moodle_database.php +++ b/lib/dml/moodle_database.php @@ -1079,9 +1079,9 @@ abstract class moodle_database { /** * Do NOT use in code, this is for use by database_manager only! - * @param string $sql query + * @param string|array $sql query or array of queries * @return bool true - * @throws dml_exception A DML specific exception is thrown for any errors. + * @throws ddl_change_structure_exception A DDL specific exception is thrown for any errors. */ public abstract function change_database_structure($sql); diff --git a/lib/dml/mssql_native_moodle_database.php b/lib/dml/mssql_native_moodle_database.php index 0a324b1c176..9b8e98fbfca 100644 --- a/lib/dml/mssql_native_moodle_database.php +++ b/lib/dml/mssql_native_moodle_database.php @@ -595,17 +595,26 @@ class mssql_native_moodle_database extends moodle_database { /** * Do NOT use in code, to be used by database_manager only! - * @param string $sql query + * @param string|array $sql query * @return bool true - * @throws dml_exception A DML specific exception is thrown for any errors. + * @throws ddl_change_structure_exception A DDL specific exception is thrown for any errors. */ public function change_database_structure($sql) { + $this->get_manager(); // Includes DDL exceptions classes ;-) + $sqls = (array)$sql; + + try { + foreach ($sqls as $sql) { + $this->query_start($sql, null, SQL_QUERY_STRUCTURE); + $result = mssql_query($sql, $this->mssql); + $this->query_end($result); + } + } catch (ddl_change_structure_exception $e) { + $this->reset_caches(); + throw $e; + } + $this->reset_caches(); - - $this->query_start($sql, null, SQL_QUERY_STRUCTURE); - $result = mssql_query($sql, $this->mssql); - $this->query_end($result); - return true; } diff --git a/lib/dml/mysqli_native_moodle_database.php b/lib/dml/mysqli_native_moodle_database.php index 3cb62a33ea5..ff6ab53444f 100644 --- a/lib/dml/mysqli_native_moodle_database.php +++ b/lib/dml/mysqli_native_moodle_database.php @@ -821,17 +821,26 @@ class mysqli_native_moodle_database extends moodle_database { /** * Do NOT use in code, to be used by database_manager only! - * @param string $sql query + * @param string|array $sql query * @return bool true - * @throws dml_exception A DML specific exception is thrown for any errors. + * @throws ddl_change_structure_exception A DDL specific exception is thrown for any errors. */ public function change_database_structure($sql) { + $this->get_manager(); // Includes DDL exceptions classes ;-) + $sqls = (array)$sql; + + try { + foreach ($sqls as $sql) { + $this->query_start($sql, null, SQL_QUERY_STRUCTURE); + $result = $this->mysqli->query($sql); + $this->query_end($result); + } + } catch (ddl_change_structure_exception $e) { + $this->reset_caches(); + throw $e; + } + $this->reset_caches(); - - $this->query_start($sql, null, SQL_QUERY_STRUCTURE); - $result = $this->mysqli->query($sql); - $this->query_end($result); - return true; } diff --git a/lib/dml/oci_native_moodle_database.php b/lib/dml/oci_native_moodle_database.php index 00f79d6576c..8e99da42b41 100644 --- a/lib/dml/oci_native_moodle_database.php +++ b/lib/dml/oci_native_moodle_database.php @@ -888,19 +888,28 @@ class oci_native_moodle_database extends moodle_database { /** * Do NOT use in code, to be used by database_manager only! - * @param string $sql query + * @param string|array $sql query * @return bool true - * @throws dml_exception A DML specific exception is thrown for any errors. + * @throws ddl_change_structure_exception A DDL specific exception is thrown for any errors. */ public function change_database_structure($sql) { + $this->get_manager(); // Includes DDL exceptions classes ;-) + $sqls = (array)$sql; + + try { + foreach ($sqls as $sql) { + $this->query_start($sql, null, SQL_QUERY_STRUCTURE); + $stmt = $this->parse_query($sql); + $result = oci_execute($stmt, $this->commit_status); + $this->query_end($result, $stmt); + oci_free_statement($stmt); + } + } catch (ddl_change_structure_exception $e) { + $this->reset_caches(); + throw $e; + } + $this->reset_caches(); - - $this->query_start($sql, null, SQL_QUERY_STRUCTURE); - $stmt = $this->parse_query($sql); - $result = oci_execute($stmt, $this->commit_status); - $this->query_end($result, $stmt); - oci_free_statement($stmt); - return true; } diff --git a/lib/dml/pdo_moodle_database.php b/lib/dml/pdo_moodle_database.php index 53fd8d5c74e..29cdd65b5de 100644 --- a/lib/dml/pdo_moodle_database.php +++ b/lib/dml/pdo_moodle_database.php @@ -174,22 +174,34 @@ abstract class pdo_moodle_database extends moodle_database { /** * Do NOT use in code, to be used by database_manager only! - * @param string $sql query - * @return bool success + * @param string|array $sql query + * @return bool true + * @throws ddl_change_structure_exception A DDL specific exception is thrown for any errors. */ public function change_database_structure($sql) { - $result = true; - $this->query_start($sql, null, SQL_QUERY_STRUCTURE); + $this->get_manager(); // Includes DDL exceptions classes ;-) + $sqls = (array)$sql; try { - $this->pdb->exec($sql); + foreach ($sqls as $sql) { + $result = true; + $this->query_start($sql, null, SQL_QUERY_STRUCTURE); + + try { + $this->pdb->exec($sql); + } catch (PDOException $ex) { + $this->lastError = $ex->getMessage(); + $result = false; + } + $this->query_end($result); + } + } catch (ddl_change_structure_exception $e) { $this->reset_caches(); - } catch (PDOException $ex) { - $this->lastError = $ex->getMessage(); - $result = false; + throw $e; } - $this->query_end($result); - return $result; + + $this->reset_caches(); + return true; } public function delete_records_select($table, $select, array $params=null) { diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php index 2ee1635843c..b87286117ca 100644 --- a/lib/dml/pgsql_native_moodle_database.php +++ b/lib/dml/pgsql_native_moodle_database.php @@ -625,18 +625,27 @@ class pgsql_native_moodle_database extends moodle_database { /** * Do NOT use in code, to be used by database_manager only! - * @param string $sql query + * @param string|array $sql query * @return bool true - * @throws dml_exception A DML specific exception is thrown for any errors. + * @throws ddl_change_structure_exception A DDL specific exception is thrown for any errors. */ public function change_database_structure($sql) { + $this->get_manager(); // Includes DDL exceptions classes ;-) + $sqls = (array)$sql; + + try { + foreach ($sqls as $sql) { + $this->query_start($sql, null, SQL_QUERY_STRUCTURE); + $result = pg_query($this->pgsql, $sql); + $this->query_end($result); + pg_free_result($result); + } + } catch (ddl_change_structure_exception $e) { + $this->reset_caches(); + throw $e; + } + $this->reset_caches(); - - $this->query_start($sql, null, SQL_QUERY_STRUCTURE); - $result = pg_query($this->pgsql, $sql); - $this->query_end($result); - - pg_free_result($result); return true; } diff --git a/lib/dml/sqlsrv_native_moodle_database.php b/lib/dml/sqlsrv_native_moodle_database.php index f6cab36050f..5110a32dd88 100644 --- a/lib/dml/sqlsrv_native_moodle_database.php +++ b/lib/dml/sqlsrv_native_moodle_database.php @@ -669,17 +669,26 @@ class sqlsrv_native_moodle_database extends moodle_database { /** * Do NOT use in code, to be used by database_manager only! - * @param string $sql query + * @param string|array $sql query * @return bool true - * @throws dml_exception A DML specific exception is thrown for any errors. + * @throws ddl_change_structure_exception A DDL specific exception is thrown for any errors. */ public function change_database_structure($sql) { + $this->get_manager(); // Includes DDL exceptions classes ;-) + $sqls = (array)$sql; + + try { + foreach ($sqls as $sql) { + $this->query_start($sql, null, SQL_QUERY_STRUCTURE); + $result = sqlsrv_query($this->sqlsrv, $sql); + $this->query_end($result); + } + } catch (ddl_change_structure_exception $e) { + $this->reset_caches(); + throw $e; + } + $this->reset_caches(); - - $this->query_start($sql, null, SQL_QUERY_STRUCTURE); - $result = sqlsrv_query($this->sqlsrv, $sql); - $this->query_end($result); - return true; } From ab3fc898f4a6c508c8cea8b19d34db5caac05737 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0koda?= Date: Fri, 24 Jan 2014 13:46:30 +0800 Subject: [PATCH 2/3] MDL-43761 use single query in mysql when crating tables --- lib/ddl/mysql_sql_generator.php | 58 +++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/lib/ddl/mysql_sql_generator.php b/lib/ddl/mysql_sql_generator.php index bbd6de2a96e..7c11f47b429 100644 --- a/lib/ddl/mysql_sql_generator.php +++ b/lib/ddl/mysql_sql_generator.php @@ -124,22 +124,66 @@ class mysql_sql_generator extends sql_generator { $sqlarr = parent::getCreateTableSQL($xmldb_table); - // Let's inject the extra MySQL tweaks. - foreach ($sqlarr as $i=>$sql) { - if (strpos($sql, 'CREATE TABLE ') === 0) { + // This is a very nasty hack that tries to use just one query per created table + // because MySQL is stupidly slow when modifying empty tables. + // Note: it is safer to inject everything on new lines because there might be some trailing -- comments. + $sqls = array(); + $prevcreate = null; + $matches = null; + foreach ($sqlarr as $sql) { + if (preg_match('/^CREATE TABLE ([^ ]+)/', $sql, $matches)) { + $prevcreate = $matches[1]; + $sql = preg_replace('/\s*\)\s*$/s', '/*keyblock*/)', $sql); + // Let's inject the extra MySQL tweaks here. if ($engine) { - $sqlarr[$i] .= " ENGINE = $engine"; + $sql .= "\n ENGINE = $engine"; } if ($collation) { if (strpos($collation, 'utf8_') === 0) { - $sqlarr[$i] .= " DEFAULT CHARACTER SET utf8"; + $sql .= "\n DEFAULT CHARACTER SET utf8"; + } + $sql .= "\n DEFAULT COLLATE = $collation"; + } + $sqls[] = $sql; + continue; + } + if ($prevcreate) { + if (preg_match('/^ALTER TABLE '.$prevcreate.' COMMENT=(.*)$/s', $sql, $matches)) { + $prev = array_pop($sqls); + $prev .= "\n COMMENT=$matches[1]"; + $sqls[] = $prev; + continue; + } + if (preg_match('/^CREATE INDEX ([^ ]+) ON '.$prevcreate.' (.*)$/s', $sql, $matches)) { + $prev = array_pop($sqls); + if (strpos($prev, '/*keyblock*/')) { + $prev = str_replace('/*keyblock*/', "\n, KEY $matches[1] $matches[2]/*keyblock*/", $prev); + $sqls[] = $prev; + continue; + } else { + $sqls[] = $prev; + } + } + if (preg_match('/^CREATE UNIQUE INDEX ([^ ]+) ON '.$prevcreate.' (.*)$/s', $sql, $matches)) { + $prev = array_pop($sqls); + if (strpos($prev, '/*keyblock*/')) { + $prev = str_replace('/*keyblock*/', "\n, UNIQUE KEY $matches[1] $matches[2]/*keyblock*/", $prev); + $sqls[] = $prev; + continue; + } else { + $sqls[] = $prev; } - $sqlarr[$i] .= " DEFAULT COLLATE = $collation"; } } + $prevcreate = null; + $sqls[] = $sql; } - return $sqlarr; + foreach ($sqls as $key => $sql) { + $sqls[$key] = str_replace('/*keyblock*/', "\n", $sql); + } + + return $sqls; } /** From b4b03d38b7e198ecfa04cfaed870a97486bd6891 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0koda?= Date: Fri, 24 Jan 2014 13:47:22 +0800 Subject: [PATCH 3/3] MDL-43761 execute bulk sql when modifying db structure In case of postgresql this uses transactions to allow easier recovery from broken installs. --- lib/dml/mysqli_native_moodle_database.php | 23 ++++++++++++++++++----- lib/dml/pgsql_native_moodle_database.php | 23 ++++++++++++++++------- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/lib/dml/mysqli_native_moodle_database.php b/lib/dml/mysqli_native_moodle_database.php index ff6ab53444f..5d2765786d2 100644 --- a/lib/dml/mysqli_native_moodle_database.php +++ b/lib/dml/mysqli_native_moodle_database.php @@ -37,6 +37,7 @@ require_once(__DIR__.'/mysqli_native_moodle_temptables.php'); */ class mysqli_native_moodle_database extends moodle_database { + /** @var mysqli $mysqli */ protected $mysqli = null; private $transactions_supported = null; @@ -827,15 +828,27 @@ class mysqli_native_moodle_database extends moodle_database { */ public function change_database_structure($sql) { $this->get_manager(); // Includes DDL exceptions classes ;-) - $sqls = (array)$sql; + if (is_array($sql)) { + $sql = implode("\n;\n", $sql); + } try { - foreach ($sqls as $sql) { - $this->query_start($sql, null, SQL_QUERY_STRUCTURE); - $result = $this->mysqli->query($sql); - $this->query_end($result); + $this->query_start($sql, null, SQL_QUERY_STRUCTURE); + $result = $this->mysqli->multi_query($sql); + if ($result === false) { + $this->query_end(false); } + while ($this->mysqli->more_results()) { + $result = $this->mysqli->next_result(); + if ($result === false) { + $this->query_end(false); + } + } + $this->query_end(true); } catch (ddl_change_structure_exception $e) { + while (@$this->mysqli->more_results()) { + @$this->mysqli->next_result(); + } $this->reset_caches(); throw $e; } diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php index b87286117ca..b0b6552ed04 100644 --- a/lib/dml/pgsql_native_moodle_database.php +++ b/lib/dml/pgsql_native_moodle_database.php @@ -37,6 +37,7 @@ require_once(__DIR__.'/pgsql_native_moodle_temptables.php'); */ class pgsql_native_moodle_database extends moodle_database { + /** @var resource $pgsql database resource */ protected $pgsql = null; protected $bytea_oid = null; @@ -631,16 +632,24 @@ class pgsql_native_moodle_database extends moodle_database { */ public function change_database_structure($sql) { $this->get_manager(); // Includes DDL exceptions classes ;-) - $sqls = (array)$sql; + if (is_array($sql)) { + $sql = implode("\n;\n", $sql); + } + if (!$this->is_transaction_started()) { + // It is better to do all or nothing, this helps with recovery... + $sql = "BEGIN ISOLATION LEVEL SERIALIZABLE;\n$sql\n; COMMIT"; + } try { - foreach ($sqls as $sql) { - $this->query_start($sql, null, SQL_QUERY_STRUCTURE); - $result = pg_query($this->pgsql, $sql); - $this->query_end($result); - pg_free_result($result); - } + $this->query_start($sql, null, SQL_QUERY_STRUCTURE); + $result = pg_query($this->pgsql, $sql); + $this->query_end($result); + pg_free_result($result); } catch (ddl_change_structure_exception $e) { + if (!$this->is_transaction_started()) { + $result = @pg_query($this->pgsql, "ROLLBACK"); + @pg_free_result($result); + } $this->reset_caches(); throw $e; }