From 934c2ee1b67acd500bb2efc1befd93b31a59aab8 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Wed, 22 Jan 2020 12:21:30 +1100 Subject: [PATCH] MDL-58584 core_ddl: getDropTableSQL should not do more than what it says --- lib/ddl/database_manager.php | 2 + lib/ddl/mssql_sql_generator.php | 15 ------- lib/ddl/mysql_sql_generator.php | 1 - lib/ddl/oracle_sql_generator.php | 1 - lib/ddl/postgres_sql_generator.php | 15 ------- lib/ddl/sql_generator.php | 13 +++++- lib/ddl/tests/ddl_test.php | 68 ++++++++++++++++++++++++++++++ lib/upgrade.txt | 1 + 8 files changed, 83 insertions(+), 33 deletions(-) diff --git a/lib/ddl/database_manager.php b/lib/ddl/database_manager.php index 7ddf633691d..c74b839c27b 100644 --- a/lib/ddl/database_manager.php +++ b/lib/ddl/database_manager.php @@ -330,6 +330,8 @@ class database_manager { throw new ddl_exception('ddlunknownerror', null, 'table drop sql not generated'); } $this->execute_sql_arr($sqlarr, array($xmldb_table->getName())); + + $this->generator->cleanup_after_drop($xmldb_table); } /** diff --git a/lib/ddl/mssql_sql_generator.php b/lib/ddl/mssql_sql_generator.php index ff672ed84fc..8c0bd93143e 100644 --- a/lib/ddl/mssql_sql_generator.php +++ b/lib/ddl/mssql_sql_generator.php @@ -183,21 +183,6 @@ class mssql_sql_generator extends sql_generator { return $sqlarr; } - /** - * Given one correct xmldb_table, returns the SQL statements - * to drop it (inside one array). - * - * @param xmldb_table $xmldb_table The table to drop. - * @return array SQL statement(s) for dropping the specified table. - */ - public function getDropTableSQL($xmldb_table) { - $sqlarr = parent::getDropTableSQL($xmldb_table); - if ($this->temptables->is_temptable($xmldb_table->getName())) { - $this->temptables->delete_temptable($xmldb_table->getName()); - } - return $sqlarr; - } - /** * Given one XMLDB Type, length and decimals, returns the DB proper SQL type. * diff --git a/lib/ddl/mysql_sql_generator.php b/lib/ddl/mysql_sql_generator.php index d768ca521e3..def4c20ed8b 100644 --- a/lib/ddl/mysql_sql_generator.php +++ b/lib/ddl/mysql_sql_generator.php @@ -384,7 +384,6 @@ class mysql_sql_generator extends sql_generator { $sqlarr = parent::getDropTableSQL($xmldb_table); if ($this->temptables->is_temptable($xmldb_table->getName())) { $sqlarr = preg_replace('/^DROP TABLE/', "DROP TEMPORARY TABLE", $sqlarr); - $this->temptables->delete_temptable($xmldb_table->getName()); } return $sqlarr; } diff --git a/lib/ddl/oracle_sql_generator.php b/lib/ddl/oracle_sql_generator.php index 4fb03bd304b..e5013168f63 100644 --- a/lib/ddl/oracle_sql_generator.php +++ b/lib/ddl/oracle_sql_generator.php @@ -211,7 +211,6 @@ class oracle_sql_generator extends sql_generator { $sqlarr = parent::getDropTableSQL($xmldb_table); if ($this->temptables->is_temptable($xmldb_table->getName())) { array_unshift($sqlarr, "TRUNCATE TABLE ". $this->getTableName($xmldb_table)); // oracle requires truncate before being able to drop a temp table - $this->temptables->delete_temptable($xmldb_table->getName()); } return $sqlarr; } diff --git a/lib/ddl/postgres_sql_generator.php b/lib/ddl/postgres_sql_generator.php index eb2685e1192..eeb3354a082 100644 --- a/lib/ddl/postgres_sql_generator.php +++ b/lib/ddl/postgres_sql_generator.php @@ -103,21 +103,6 @@ class postgres_sql_generator extends sql_generator { return $sqlarr; } - /** - * Given one correct xmldb_table, returns the SQL statements - * to drop it (inside one array). - * - * @param xmldb_table $xmldb_table The table to drop. - * @return array SQL statement(s) for dropping the specified table. - */ - public function getDropTableSQL($xmldb_table) { - $sqlarr = parent::getDropTableSQL($xmldb_table); - if ($this->temptables->is_temptable($xmldb_table->getName())) { - $this->temptables->delete_temptable($xmldb_table->getName()); - } - return $sqlarr; - } - /** * Given one correct xmldb_index, returns the SQL statements * needed to create it (in array). diff --git a/lib/ddl/sql_generator.php b/lib/ddl/sql_generator.php index 9ad419f512d..99fd21f1390 100644 --- a/lib/ddl/sql_generator.php +++ b/lib/ddl/sql_generator.php @@ -653,7 +653,7 @@ abstract class sql_generator { } /** - * Given one correct xmldb_table and the new name, returns the SQL statements + * Given one correct xmldb_table, returns the SQL statements * to drop it (inside one array). Works also for temporary tables. * * @param xmldb_table $xmldb_table The table to drop. @@ -674,6 +674,17 @@ abstract class sql_generator { return $results; } + /** + * Performs any clean up that needs to be done after a table is dropped. + * + * @param xmldb_table $table + */ + public function cleanup_after_drop(xmldb_table $table): void { + if ($this->temptables->is_temptable($table->getName())) { + $this->temptables->delete_temptable($table->getName()); + } + } + /** * Given one xmldb_table and one xmldb_field, return the SQL statements needed to add the field to the table. * diff --git a/lib/ddl/tests/ddl_test.php b/lib/ddl/tests/ddl_test.php index a913ce72e4d..06fabd197b1 100644 --- a/lib/ddl/tests/ddl_test.php +++ b/lib/ddl/tests/ddl_test.php @@ -1828,6 +1828,74 @@ class core_ddl_testcase extends database_driver_testcase { $this->assertFalse($dbman->table_exists('test_table1')); } + /** + * get_columns should return an empty array for ex-temptables. + */ + public function test_leftover_temp_tables_columns() { + $DB = $this->tdb; // Do not use global $DB! + $dbman = $this->tdb->get_manager(); + + // Create temp table0. + $table0 = $this->tables['test_table0']; + $dbman->create_temp_table($table0); + + $dbman->drop_table($table0); + + // Get columns and perform some basic tests. + $columns = $DB->get_columns('test_table0'); + $this->assertEquals([], $columns); + } + + /** + * Deleting a temp table should not purge the whole cache + */ + public function test_leftover_temp_tables_cache() { + $DB = $this->tdb; // Do not use global $DB! + $dbman = $this->tdb->get_manager(); + + // Create 2 temp tables. + $table0 = $this->tables['test_table0']; + $dbman->create_temp_table($table0); + $table1 = $this->tables['test_table1']; + $dbman->create_temp_table($table1); + + // Create a normal table. + $table2 = new xmldb_table ('test_table2'); + $table2->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); + $table2->add_field('course', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0'); + $table2->add_key('primary', XMLDB_KEY_PRIMARY, array('id')); + $table2->setComment("This is a test'n drop table. You can drop it safely"); + $this->tables[$table2->getName()] = $table2; + $dbman->create_table($table2); + + // Get columns for the tables, so that relevant caches are populated with their data. + $DB->get_columns('test_table0'); + $DB->get_columns('test_table1'); + $DB->get_columns('test_table2'); + + $dbman->drop_table($table0); + + $rc = new ReflectionClass('moodle_database'); + $rcm = $rc->getMethod('get_temp_tables_cache'); + $rcm->setAccessible(true); + $metacachetemp = $rcm->invokeArgs($DB, []); + + // Data of test_table0 should be removed from the cache. + $this->assertEquals(false, $metacachetemp->has('test_table0')); + + // Data of test_table1 should be intact. + $this->assertEquals(true, $metacachetemp->has('test_table1')); + + $rc = new ReflectionClass('moodle_database'); + $rcm = $rc->getMethod('get_metacache'); + $rcm->setAccessible(true); + $metacache = $rcm->invokeArgs($DB, []); + + // Data of test_table2 should be intact. + $this->assertEquals(true, $metacache->has('test_table2')); + + } + public function test_reset_sequence() { $DB = $this->tdb; $dbman = $DB->get_manager(); diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 0f496adc075..12816d5b0b5 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -30,6 +30,7 @@ information provided here is intended especially for developers. - mod_quiz * The database drivers (moodle_database and subclasses) don't need to implement get_columns() anymore. They have to implement fetch_columns instead. +* Added function cleanup_after_drop to the database_manager class to take care of all the cleanups that need to be done after a table is dropped. === 3.8 === * Add CLI option to notify all cron tasks to stop: admin/cli/cron.php --stop