From ed00d67c99e84aa21dde93a524b243e56d258ed1 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Tue, 10 Oct 2017 15:08:54 +0100 Subject: [PATCH 1/2] MDL-60174 core_dml: get_recordset on Postgres eats all the RAM On Postgres, get_recordset_sql loads all the results into memory (within the Postgres library, which doesn't count towards the PHP memory limit, but does count towards making your server run out of memory) as soon as the query completes. This commit changes the code to use cursors, which in Postgres allow the results to be returned in smaller chunks (by default 100,000 rows). --- lib/dml/pgsql_native_moodle_database.php | 97 +++- lib/dml/pgsql_native_moodle_recordset.php | 68 ++- lib/dml/tests/pgsql_native_recordset_test.php | 432 ++++++++++++++++++ 3 files changed, 583 insertions(+), 14 deletions(-) create mode 100644 lib/dml/tests/pgsql_native_recordset_test.php diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php index 44d298c03a6..b6114e5d00d 100644 --- a/lib/dml/pgsql_native_moodle_database.php +++ b/lib/dml/pgsql_native_moodle_database.php @@ -45,6 +45,12 @@ class pgsql_native_moodle_database extends moodle_database { /** @var bool savepoint hack for MDL-35506 - workaround for automatic transaction rollback on error */ protected $savepointpresent = false; + /** @var int Number of cursors used (for constructing a unique ID) */ + protected $cursorcount = 0; + + /** @var int Default number of rows to fetch at a time when using recordsets with cursors */ + const DEFAULT_FETCH_BUFFER_SIZE = 100000; + /** * Detects if all needed PHP stuff installed. * Note: can be used before connect() @@ -734,14 +740,89 @@ class pgsql_native_moodle_database extends moodle_database { list($sql, $params, $type) = $this->fix_sql_params($sql, $params); $this->query_start($sql, $params, SQL_QUERY_SELECT); - $result = pg_query_params($this->pgsql, $sql, $params); - $this->query_end($result); - return $this->create_recordset($result); + // For any query that doesn't explicitly specify a limit, we must use cursors to stop it + // loading the entire thing (unless the config setting is turned off). + $usecursors = !$limitnum && ($this->get_fetch_buffer_size() > 0); + if ($usecursors) { + // Work out the cursor unique identifer. This is based on a simple count used which + // should be OK because the identifiers only need to be unique within the current + // transaction. + $this->cursorcount++; + $cursorname = 'crs' . $this->cursorcount; + + // Do the query to a cursor. + $sql = 'DECLARE ' . $cursorname . ' NO SCROLL CURSOR WITH HOLD FOR ' . $sql; + $result = pg_query_params($this->pgsql, $sql, $params); + } else { + $result = pg_query_params($this->pgsql, $sql, $params); + $cursorname = ''; + } + + $this->query_end($result); + if ($usecursors) { + pg_free_result($result); + $result = null; + } + + return new pgsql_native_moodle_recordset($result, $this, $cursorname); } - protected function create_recordset($result) { - return new pgsql_native_moodle_recordset($result); + /** + * Gets size of fetch buffer used for recordset queries. + * + * If this returns 0 then cursors will not be used, meaning recordset queries will occupy enough + * memory as needed for the Postgres library to hold the entire query results in memory. + * + * @return int Fetch buffer size or 0 indicating not to use cursors + */ + protected function get_fetch_buffer_size() { + if (array_key_exists('fetchbuffersize', $this->dboptions)) { + return (int)$this->dboptions['fetchbuffersize']; + } else { + return self::DEFAULT_FETCH_BUFFER_SIZE; + } + } + + /** + * Retrieves data from cursor. For use by recordset only; do not call directly. + * + * Return value contains the next batch of Postgres data, and a boolean indicating if this is + * definitely the last batch (if false, there may be more) + * + * @param string $cursorname Name of cursor to read from + * @return array Array with 2 elements (next data batch and boolean indicating last batch) + */ + public function fetch_from_cursor($cursorname) { + $count = $this->get_fetch_buffer_size(); + + $sql = 'FETCH ' . $count . ' FROM ' . $cursorname; + + $this->query_start($sql, [], SQL_QUERY_AUX); + $result = pg_query($this->pgsql, $sql); + $last = pg_num_rows($result) !== $count; + + $this->query_end($result); + + return [$result, $last]; + } + + /** + * Closes a cursor. For use by recordset only; do not call directly. + * + * @param string $cursorname Name of cursor to close + * @return bool True if we actually closed one, false if the transaction was cancelled + */ + public function close_cursor($cursorname) { + // If the transaction got cancelled, then ignore this request. + $sql = 'CLOSE ' . $cursorname; + $this->query_start($sql, [], SQL_QUERY_AUX); + $result = pg_query($this->pgsql, $sql); + $this->query_end($result); + if ($result) { + pg_free_result($result); + } + return true; } /** @@ -1366,7 +1447,7 @@ class pgsql_native_moodle_database extends moodle_database { protected function begin_transaction() { $this->savepointpresent = true; $sql = "BEGIN ISOLATION LEVEL READ COMMITTED; SAVEPOINT moodle_pg_savepoint"; - $this->query_start($sql, NULL, SQL_QUERY_AUX); + $this->query_start($sql, null, SQL_QUERY_AUX); $result = pg_query($this->pgsql, $sql); $this->query_end($result); @@ -1381,7 +1462,7 @@ class pgsql_native_moodle_database extends moodle_database { protected function commit_transaction() { $this->savepointpresent = false; $sql = "RELEASE SAVEPOINT moodle_pg_savepoint; COMMIT"; - $this->query_start($sql, NULL, SQL_QUERY_AUX); + $this->query_start($sql, null, SQL_QUERY_AUX); $result = pg_query($this->pgsql, $sql); $this->query_end($result); @@ -1396,7 +1477,7 @@ class pgsql_native_moodle_database extends moodle_database { protected function rollback_transaction() { $this->savepointpresent = false; $sql = "RELEASE SAVEPOINT moodle_pg_savepoint; ROLLBACK"; - $this->query_start($sql, NULL, SQL_QUERY_AUX); + $this->query_start($sql, null, SQL_QUERY_AUX); $result = pg_query($this->pgsql, $sql); $this->query_end($result); diff --git a/lib/dml/pgsql_native_moodle_recordset.php b/lib/dml/pgsql_native_moodle_recordset.php index dc99f5b249b..a6eed21d030 100644 --- a/lib/dml/pgsql_native_moodle_recordset.php +++ b/lib/dml/pgsql_native_moodle_recordset.php @@ -40,26 +40,64 @@ class pgsql_native_moodle_recordset extends moodle_recordset { protected $current; protected $blobs = array(); + /** @var string Name of cursor or '' if none */ + protected $cursorname; + + /** @var pgsql_native_moodle_database Postgres database resource */ + protected $db; + + /** @var bool True if there are no more rows to fetch from the cursor */ + protected $lastbatch; + /** * Build a new recordset to iterate over. * - * @param resource $result A pg_query() result object to create a recordset from. + * When using cursors, $result will be null initially. + * + * @param resource|null $result A pg_query() result object to create a recordset from. + * @param pgsql_native_moodle_database $db Database object (only required when using cursors) + * @param string $cursorname Name of cursor or '' if none */ - public function __construct($result) { + public function __construct($result, pgsql_native_moodle_database $db = null, $cursorname = '') { + if ($cursorname && !$db) { + throw new coding_exception('When specifying a cursor, $db is required'); + } $this->result = $result; + $this->db = $db; + $this->cursorname = $cursorname; + + // When there is a cursor, do the initial fetch. + if ($cursorname) { + $this->fetch_cursor_block(); + } // Find out if there are any blobs. - $numfields = pg_num_fields($result); + $numfields = pg_num_fields($this->result); for ($i = 0; $i < $numfields; $i++) { - $type = pg_field_type($result, $i); + $type = pg_field_type($this->result, $i); if ($type == 'bytea') { - $this->blobs[] = pg_field_name($result, $i); + $this->blobs[] = pg_field_name($this->result, $i); } } $this->current = $this->fetch_next(); } + /** + * Fetches the next block of data when using cursors. + * + * @throws coding_exception If you call this when the fetch buffer wasn't freed yet + */ + protected function fetch_cursor_block() { + if ($this->result) { + throw new coding_exception('Unexpected non-empty result when fetching from cursor'); + } + list($this->result, $this->lastbatch) = $this->db->fetch_from_cursor($this->cursorname); + if (!$this->result) { + throw new coding_exception('Unexpected failure when fetching from cursor'); + } + } + public function __destruct() { $this->close(); } @@ -69,9 +107,21 @@ class pgsql_native_moodle_recordset extends moodle_recordset { return false; } if (!$row = pg_fetch_assoc($this->result)) { + // There are no more rows in this result. pg_free_result($this->result); $this->result = null; - return false; + + // If using a cursor, can we fetch the next block? + if ($this->cursorname && !$this->lastbatch) { + $this->fetch_cursor_block(); + if (!$row = pg_fetch_assoc($this->result)) { + pg_free_result($this->result); + $this->result = null; + return false; + } + } else { + return false; + } } if ($this->blobs) { @@ -111,5 +161,11 @@ class pgsql_native_moodle_recordset extends moodle_recordset { } $this->current = null; $this->blobs = null; + + // If using cursors, close the cursor. + if ($this->cursorname) { + $this->db->close_cursor($this->cursorname); + $this->cursorname = null; + } } } diff --git a/lib/dml/tests/pgsql_native_recordset_test.php b/lib/dml/tests/pgsql_native_recordset_test.php new file mode 100644 index 00000000000..07ebe6f3c80 --- /dev/null +++ b/lib/dml/tests/pgsql_native_recordset_test.php @@ -0,0 +1,432 @@ +. + +/** + * Test specific features of the Postgres dml support relating to recordsets. + * + * @package core + * @category test + * @copyright 2017 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot.'/lib/dml/pgsql_native_moodle_database.php'); + +/** + * Test specific features of the Postgres dml support relating to recordsets. + * + * @package core + * @category test + * @copyright 2017 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class pgsql_native_recordset_testcase extends basic_testcase { + + /** @var pgsql_native_moodle_database Special database connection */ + protected $specialdb; + + /** + * Creates a second db connection and a temp table with values in for testing. + */ + protected function setUp() { + global $DB; + + parent::setUp(); + + // Skip tests if not using Postgres. + if (!($DB instanceof pgsql_native_moodle_database)) { + $this->markTestSkipped('Postgres-only test'); + } + } + + /** + * Initialises database connection with given fetch buffer size + * @param int $fetchbuffersize Size of fetch buffer + */ + protected function init_db($fetchbuffersize) { + global $CFG, $DB; + + // To make testing easier, create a database with the same dboptions as the real one, + // but a low number for the cursor size. + $this->specialdb = \moodle_database::get_driver_instance('pgsql', 'native', true); + $dboptions = ['fetchbuffersize' => $fetchbuffersize]; + $this->specialdb->connect($CFG->dbhost, $CFG->dbuser, $CFG->dbpass, $CFG->dbname, + $DB->get_prefix(), $dboptions); + + // Create a temp table. + $dbman = $this->specialdb->get_manager(); + $table = new xmldb_table('silly_test_table'); + $table->add_field('id', XMLDB_TYPE_INTEGER, 10, null, XMLDB_NOTNULL, XMLDB_SEQUENCE); + $table->add_field('msg', XMLDB_TYPE_CHAR, 255); + $table->add_key('primary', XMLDB_KEY_PRIMARY, ['id']); + $dbman->create_temp_table($table); + + // Add some records to the table. + for ($index = 1; $index <= 7; $index++) { + $this->specialdb->insert_record('silly_test_table', ['msg' => 'record' . $index]); + } + } + + /** + * Gets rid of the second db connection. + */ + protected function tearDown() { + if ($this->specialdb) { + $table = new xmldb_table('silly_test_table'); + $this->specialdb->get_manager()->drop_table($table); + $this->specialdb->dispose(); + $this->specialdb = null; + } + parent::tearDown(); + } + + /** + * Tests that get_recordset_sql works when using cursors, which it does when no limit is + * specified. + */ + public function test_recordset_cursors() { + $this->init_db(3); + + // Query the table and check the actual queries using debug mode, also check the count. + $this->specialdb->set_debug(true); + $before = $this->specialdb->perf_get_queries(); + ob_start(); + $rs = $this->specialdb->get_recordset_sql('SELECT * FROM {silly_test_table} ORDER BY id'); + $index = 0; + foreach ($rs as $rec) { + $index++; + $this->assertEquals('record' . $index, $rec->msg); + } + $this->assertEquals(7, $index); + $rs->close(); + $debugging = ob_get_contents(); + ob_end_clean(); + + // Expect 4 fetches - first three, next three, last one (with 2). + $this->assert_query_regexps([ + '~SELECT \* FROM~', + '~FETCH 3 FROM crs1~', + '~FETCH 3 FROM crs1~', + '~FETCH 3 FROM crs1~', + '~CLOSE crs1~'], $debugging); + + // There should have been 7 queries tracked for perf log. + $this->assertEquals(5, $this->specialdb->perf_get_queries() - $before); + + // Try a second time - this time we'll request exactly 3 items so that it has to query + // twice (as it can't tell if the first batch is the last). + $before = $this->specialdb->perf_get_queries(); + ob_start(); + $rs = $this->specialdb->get_recordset_sql( + 'SELECT * FROM {silly_test_table} WHERE id <= ? ORDER BY id', [3]); + $index = 0; + foreach ($rs as $rec) { + $index++; + $this->assertEquals('record' . $index, $rec->msg); + } + $this->assertEquals(3, $index); + $rs->close(); + $debugging = ob_get_contents(); + ob_end_clean(); + + $this->specialdb->set_debug(false); + + // Expect 2 fetches - first three, then next one (empty). + $this->assert_query_regexps([ + '~SELECT \* FROM~', + '~FETCH 3 FROM crs2~', + '~FETCH 3 FROM crs2~', + '~CLOSE crs2~'], $debugging); + + // There should have been 4 queries tracked for perf log. + $this->assertEquals(4, $this->specialdb->perf_get_queries() - $before); + } + + /** + * Tests that get_recordset_sql works when using cursors and when there are two overlapping + * recordsets being used. + */ + public function test_recordset_cursors_overlapping() { + $this->init_db(3); + + $rs1 = $this->specialdb->get_recordset('silly_test_table', null, 'id'); + $rs2 = $this->specialdb->get_recordset('silly_test_table', null, 'id DESC'); + + // Read first 3 from first recordset. + $read = []; + $read[] = $rs1->current()->id; + $rs1->next(); + $read[] = $rs1->current()->id; + $rs1->next(); + $read[] = $rs1->current()->id; + $rs1->next(); + $this->assertEquals([1, 2, 3], $read); + + // Read 5 from second recordset. + $read = []; + $read[] = $rs2->current()->id; + $rs2->next(); + $read[] = $rs2->current()->id; + $rs2->next(); + $read[] = $rs2->current()->id; + $rs2->next(); + $read[] = $rs2->current()->id; + $rs2->next(); + $read[] = $rs2->current()->id; + $rs2->next(); + $this->assertEquals([7, 6, 5, 4, 3], $read); + + // Now read remainder of first recordset and close it. + $read = []; + $read[] = $rs1->current()->id; + $rs1->next(); + $read[] = $rs1->current()->id; + $rs1->next(); + $read[] = $rs1->current()->id; + $rs1->next(); + $read[] = $rs1->current()->id; + $rs1->next(); + $this->assertFalse($rs1->valid()); + $this->assertEquals([4, 5, 6, 7], $read); + $rs1->close(); + + // And remainder of second. + $read = []; + $read[] = $rs2->current()->id; + $rs2->next(); + $read[] = $rs2->current()->id; + $rs2->next(); + $this->assertFalse($rs2->valid()); + $this->assertEquals([2, 1], $read); + $rs2->close(); + } + + /** + * Tests that get_recordset_sql works when using cursors and transactions inside. + */ + public function test_recordset_cursors_transaction_inside() { + $this->init_db(3); + + // Transaction inside the recordset processing. + $rs = $this->specialdb->get_recordset('silly_test_table', null, 'id'); + $read = []; + foreach ($rs as $rec) { + $read[] = $rec->id; + $transaction = $this->specialdb->start_delegated_transaction(); + $transaction->allow_commit(); + } + $this->assertEquals([1, 2, 3, 4, 5, 6, 7], $read); + $rs->close(); + } + + /** + * Tests that get_recordset_sql works when using cursors and a transaction outside. + */ + public function test_recordset_cursors_transaction_outside() { + $this->init_db(3); + + // Transaction outside the recordset processing. + $transaction = $this->specialdb->start_delegated_transaction(); + $rs = $this->specialdb->get_recordset('silly_test_table', null, 'id'); + $read = []; + foreach ($rs as $rec) { + $read[] = $rec->id; + } + $this->assertEquals([1, 2, 3, 4, 5, 6, 7], $read); + $rs->close(); + $transaction->allow_commit(); + } + + /** + * Tests that get_recordset_sql works when using cursors and a transaction overlapping. + */ + public function test_recordset_cursors_transaction_overlapping_before() { + $this->init_db(3); + + // Transaction outside the recordset processing. + $transaction = $this->specialdb->start_delegated_transaction(); + $rs = $this->specialdb->get_recordset('silly_test_table', null, 'id'); + $transaction->allow_commit(); + $read = []; + foreach ($rs as $rec) { + $read[] = $rec->id; + } + $this->assertEquals([1, 2, 3, 4, 5, 6, 7], $read); + $rs->close(); + } + + /** + * Tests that get_recordset_sql works when using cursors and a transaction overlapping. + */ + public function test_recordset_cursors_transaction_overlapping_after() { + $this->init_db(3); + + // Transaction outside the recordset processing. + $rs = $this->specialdb->get_recordset('silly_test_table', null, 'id'); + $transaction = $this->specialdb->start_delegated_transaction(); + $read = []; + foreach ($rs as $rec) { + $read[] = $rec->id; + } + $this->assertEquals([1, 2, 3, 4, 5, 6, 7], $read); + $rs->close(); + $transaction->allow_commit(); + } + + /** + * Tests that get_recordset_sql works when using cursors and a transaction that 'fails' and gets + * rolled back. + */ + public function test_recordset_cursors_transaction_rollback() { + $this->init_db(3); + + try { + $rs = $this->specialdb->get_recordset('silly_test_table', null, 'id'); + $transaction = $this->specialdb->start_delegated_transaction(); + $this->specialdb->delete_records('silly_test_table', ['id' => 5]); + $transaction->rollback(new dml_transaction_exception('rollback please')); + $this->fail('should not get here'); + } catch (dml_transaction_exception $e) { + $this->assertContains('rollback please', $e->getMessage()); + } finally { + + // Rollback should not kill our recordset. + $read = []; + foreach ($rs as $rec) { + $read[] = $rec->id; + } + $this->assertEquals([1, 2, 3, 4, 5, 6, 7], $read); + + // This would happen in real code (that isn't within the same function) anyway because + // it would go out of scope. + $rs->close(); + } + + // OK, transaction aborted, now get the recordset again and check nothing was deleted. + $rs = $this->specialdb->get_recordset('silly_test_table', null, 'id'); + $read = []; + foreach ($rs as $rec) { + $read[] = $rec->id; + } + $this->assertEquals([1, 2, 3, 4, 5, 6, 7], $read); + $rs->close(); + } + + /** + * Tests that get_recordset_sql works when not using cursors, because a limit is specified. + */ + public function test_recordset_no_cursors_limit() { + $this->init_db(3); + + $this->specialdb->set_debug(true); + $before = $this->specialdb->perf_get_queries(); + ob_start(); + $rs = $this->specialdb->get_recordset_sql( + 'SELECT * FROM {silly_test_table} ORDER BY id', [], 0, 100); + $index = 0; + foreach ($rs as $rec) { + $index++; + $this->assertEquals('record' . $index, $rec->msg); + } + $this->assertEquals(7, $index); + $rs->close(); + $this->specialdb->set_debug(false); + $debugging = ob_get_contents(); + ob_end_clean(); + + // Expect direct request without using cursors. + $this->assert_query_regexps(['~SELECT \* FROM~'], $debugging); + + // There should have been 1 query tracked for perf log. + $this->assertEquals(1, $this->specialdb->perf_get_queries() - $before); + } + + /** + * Tests that get_recordset_sql works when not using cursors, because the config setting turns + * them off. + */ + public function test_recordset_no_cursors_config() { + $this->init_db(0); + + $this->specialdb->set_debug(true); + $before = $this->specialdb->perf_get_queries(); + ob_start(); + $rs = $this->specialdb->get_recordset_sql('SELECT * FROM {silly_test_table} ORDER BY id'); + $index = 0; + foreach ($rs as $rec) { + $index++; + $this->assertEquals('record' . $index, $rec->msg); + } + $this->assertEquals(7, $index); + $rs->close(); + $this->specialdb->set_debug(false); + $debugging = ob_get_contents(); + ob_end_clean(); + + // Expect direct request without using cursors. + $this->assert_query_regexps(['~SELECT \* FROM~'], $debugging); + + // There should have been 1 query tracked for perf log. + $this->assertEquals(1, $this->specialdb->perf_get_queries() - $before); + } + + /** + * Asserts that database debugging output matches the expected list of SQL queries, specified + * as an array of regular expressions. + * + * @param string[] $expected Expected regular expressions + * @param string $debugging Debugging text from the database + */ + protected function assert_query_regexps(array $expected, $debugging) { + $lines = explode("\n", $debugging); + $index = 0; + $params = false; + foreach ($lines as $line) { + if ($params) { + if ($line === ')]') { + $params = false; + } + continue; + } + // Skip irrelevant lines. + if (preg_match('~^---~', $line)) { + continue; + } + if (preg_match('~^Query took~', $line)) { + continue; + } + if (trim($line) === '') { + continue; + } + // Skip param lines. + if ($line === '[array (') { + $params = true; + continue; + } + if (!array_key_exists($index, $expected)) { + $this->fail('More queries than expected'); + } + $this->assertRegExp($expected[$index++], $line); + } + if (array_key_exists($index, $expected)) { + $this->fail('Fewer queries than expected'); + } + } + +} From a938e4096c6ea0021eecc6eca6997f1d3437532e Mon Sep 17 00:00:00 2001 From: sam marshall Date: Wed, 11 Oct 2017 12:16:12 +0100 Subject: [PATCH 2/2] MDL-60174 core_dml: fix miscellaneous incorrect recordset usage The new recordset support for Postgres requires transactions and will cause errors if recordsets are not closed correctly. This commit fixes problems that were identified during unit tests, and via some basic code analysis, across all core code. Most of these are incorrect usage of recordset (forgetting to close them). --- admin/cli/fix_deleted_users.php | 1 + admin/tool/messageinbound/index.php | 1 + admin/tool/spamcleaner/index.php | 22 +++++++++++-------- analytics/classes/manager.php | 1 + backup/moodle2/restore_stepslib.php | 2 +- cohort/lib.php | 1 + grade/grading/form/lib.php | 1 + .../classes/local/screen/screen.php | 1 + group/lib.php | 3 +++ lib/blocklib.php | 1 + lib/classes/message/inbound/manager.php | 1 + lib/classes/plugininfo/portfolio.php | 3 ++- lib/db/upgrade.php | 1 + lib/filestorage/file_storage.php | 1 + lib/navigationlib.php | 1 + lib/tablelib.php | 15 +++++++++---- lib/testing/classes/util.php | 1 + message/tests/externallib_test.php | 8 +++---- mod/feedback/classes/responses_table.php | 3 +-- mod/forum/tests/subscriptions_test.php | 15 +++++++------ mod/glossary/lib.php | 2 +- mod/glossary/print.php | 4 ++++ mod/glossary/view.php | 4 ++++ mod/quiz/tests/attempts_test.php | 2 ++ mod/wiki/locallib.php | 2 +- question/classes/bank/view.php | 2 ++ report/security/locallib.php | 1 + 27 files changed, 70 insertions(+), 30 deletions(-) diff --git a/admin/cli/fix_deleted_users.php b/admin/cli/fix_deleted_users.php index d2a895057a2..cc052b4e702 100644 --- a/admin/cli/fix_deleted_users.php +++ b/admin/cli/fix_deleted_users.php @@ -73,6 +73,7 @@ foreach ($rs as $user) { echo "Redeleting user $user->id: $user->username ($user->email)\n"; delete_user($user); } +$rs->close(); cli_heading('Deleting all leftovers'); diff --git a/admin/tool/messageinbound/index.php b/admin/tool/messageinbound/index.php index 8d04d450aed..90c2a128933 100644 --- a/admin/tool/messageinbound/index.php +++ b/admin/tool/messageinbound/index.php @@ -40,6 +40,7 @@ if (empty($classname)) { foreach ($records as $record) { $instances[] = \core\message\inbound\manager::get_handler($record->classname); } + $records->close(); echo $OUTPUT->header(); echo $renderer->messageinbound_handlers_table($instances); diff --git a/admin/tool/spamcleaner/index.php b/admin/tool/spamcleaner/index.php index 63d3485e5dd..2f98bf41006 100644 --- a/admin/tool/spamcleaner/index.php +++ b/admin/tool/spamcleaner/index.php @@ -234,15 +234,19 @@ function search_spammers($keywords) { $keywordlist = implode(', ', $keywords); echo $OUTPUT->box(get_string('spamresult', 'tool_spamcleaner').s($keywordlist)).' ...'; - print_user_list(array($spamusers_desc, - $spamusers_blog, - $spamusers_blogsub, - $spamusers_comment, - $spamusers_message, - $spamusers_forumpost, - $spamusers_forumpostsub - ), - $keywords); + $recordsets = [ + $spamusers_desc, + $spamusers_blog, + $spamusers_blogsub, + $spamusers_comment, + $spamusers_message, + $spamusers_forumpost, + $spamusers_forumpostsub + ]; + print_user_list($recordsets, $keywords); + foreach ($recordsets as $rs) { + $rs->close(); + } } diff --git a/analytics/classes/manager.php b/analytics/classes/manager.php index 4b0bcac480d..cdf787e2853 100644 --- a/analytics/classes/manager.php +++ b/analytics/classes/manager.php @@ -360,6 +360,7 @@ class manager { } $existingcalculations[$calculation->indicator][$calculation->sampleid] = $calculation->value; } + $calculations->close(); return $existingcalculations; } diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index b53ffa67e18..f17a6edcc79 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -981,8 +981,8 @@ class restore_process_course_modules_availability extends restore_execution_step $DB->set_field('course_' . $table . 's', 'availability', $newvalue, array('id' => $thingid)); } + $rs->close(); } - $rs->close(); } } diff --git a/cohort/lib.php b/cohort/lib.php index 4de545e5a26..5f257cde1bb 100644 --- a/cohort/lib.php +++ b/cohort/lib.php @@ -504,6 +504,7 @@ function cohort_get_invisible_contexts() { $excludedcontexts[] = $ctx->id; } } + $records->close(); return $excludedcontexts; } diff --git a/grade/grading/form/lib.php b/grade/grading/form/lib.php index b79f5c77362..2f0666221df 100644 --- a/grade/grading/form/lib.php +++ b/grade/grading/form/lib.php @@ -428,6 +428,7 @@ abstract class gradingform_controller { foreach ($records as $record) { $rv[] = $this->get_instance($record); } + $records->close(); return $rv; } diff --git a/grade/report/singleview/classes/local/screen/screen.php b/grade/report/singleview/classes/local/screen/screen.php index 56709dfcb6f..db3ecb601e3 100644 --- a/grade/report/singleview/classes/local/screen/screen.php +++ b/grade/report/singleview/classes/local/screen/screen.php @@ -419,6 +419,7 @@ abstract class screen { while ($user = $gui->next_user()) { $users[$user->user->id] = $user->user; } + $gui->close(); return $users; } diff --git a/group/lib.php b/group/lib.php index 5df273b81fc..55ee9003456 100644 --- a/group/lib.php +++ b/group/lib.php @@ -618,6 +618,7 @@ function groups_delete_groupings_groups($courseid, $showfeedback=false) { foreach ($results as $result) { groups_unassign_grouping($result->groupingid, $result->groupid, false); } + $results->close(); // Invalidate the grouping cache for the course cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($courseid)); @@ -646,6 +647,7 @@ function groups_delete_groups($courseid, $showfeedback=false) { foreach ($groups as $group) { groups_delete_group($group); } + $groups->close(); // Invalidate the grouping cache for the course cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($courseid)); @@ -676,6 +678,7 @@ function groups_delete_groupings($courseid, $showfeedback=false) { foreach ($groupings as $grouping) { groups_delete_grouping($grouping); } + $groupings->close(); // Invalidate the grouping cache for the course. cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($courseid)); diff --git a/lib/blocklib.php b/lib/blocklib.php index 81d563ffb84..219a3782299 100644 --- a/lib/blocklib.php +++ b/lib/blocklib.php @@ -800,6 +800,7 @@ class block_manager { $unknown[] = $bi; } } + $blockinstances->close(); // Pages don't necessarily have a defaultregion. The one time this can // happen is when there are no theme block regions, but the script itself diff --git a/lib/classes/message/inbound/manager.php b/lib/classes/message/inbound/manager.php index 8534df679e4..9bc16cddb38 100644 --- a/lib/classes/message/inbound/manager.php +++ b/lib/classes/message/inbound/manager.php @@ -74,6 +74,7 @@ class manager { self::remove_messageinbound_handler($handler); } } + $existinghandlers->close(); self::create_missing_messageinbound_handlers_for_component($componentname); } diff --git a/lib/classes/plugininfo/portfolio.php b/lib/classes/plugininfo/portfolio.php index 78e94f40a96..80f57d50c77 100644 --- a/lib/classes/plugininfo/portfolio.php +++ b/lib/classes/plugininfo/portfolio.php @@ -43,6 +43,7 @@ class portfolio extends base { foreach ($rs as $repository) { $enabled[$repository->plugin] = $repository->plugin; } + $rs->close(); return $enabled; } @@ -91,4 +92,4 @@ class portfolio extends base { parent::uninstall_cleanup(); } -} \ No newline at end of file +} diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 91eb81fc9c2..88cdee7d0b9 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -1183,6 +1183,7 @@ function xmldb_main_upgrade($oldversion) { $i++; $pbar->update($i, $total, "Updating duplicate question category stamp - $i/$total."); } + $rs->close(); unset($usedstamps); // The uniqueness of each (contextid, stamp) pair is now guaranteed, so add the unique index to stop future duplicates. diff --git a/lib/filestorage/file_storage.php b/lib/filestorage/file_storage.php index 15b4ef4e58b..49960e28514 100644 --- a/lib/filestorage/file_storage.php +++ b/lib/filestorage/file_storage.php @@ -2001,6 +2001,7 @@ class file_storage { foreach ($rs as $filerecord) { $files[$filerecord->pathnamehash] = $this->get_file_instance($filerecord); } + $rs->close(); return $files; } diff --git a/lib/navigationlib.php b/lib/navigationlib.php index 7c46fc357f5..9f22f184cea 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -3258,6 +3258,7 @@ class global_navigation_for_ajax extends global_navigation { foreach ($categories as $category){ $coursesubcategories = array_merge($coursesubcategories, explode('/', trim($category->path, "/"))); } + $categories->close(); $coursesubcategories = array_unique($coursesubcategories); // Only add a subcategory if it is part of the path to user's course and diff --git a/lib/tablelib.php b/lib/tablelib.php index d1f1adc0b4e..835c751e522 100644 --- a/lib/tablelib.php +++ b/lib/tablelib.php @@ -1499,9 +1499,9 @@ class table_sql extends flexible_table { * method or if other_cols returns NULL then put the data straight into the * table. * - * @return void + * After calling this function, don't forget to call close_recordset. */ - function build_table() { + public function build_table() { if ($this->rawdata instanceof \Traversable && !$this->rawdata->valid()) { return; @@ -1515,10 +1515,16 @@ class table_sql extends flexible_table { $this->add_data_keyed($formattedrow, $this->get_row_class($row)); } + } - if ($this->rawdata instanceof \core\dml\recordset_walk || - $this->rawdata instanceof moodle_recordset) { + /** + * Closes recordset (for use after building the table). + */ + public function close_recordset() { + if ($this->rawdata && ($this->rawdata instanceof \core\dml\recordset_walk || + $this->rawdata instanceof moodle_recordset)) { $this->rawdata->close(); + $this->rawdata = null; } } @@ -1629,6 +1635,7 @@ class table_sql extends flexible_table { $this->setup(); $this->query_db($pagesize, $useinitialsbar); $this->build_table(); + $this->close_recordset(); $this->finish_output(); } } diff --git a/lib/testing/classes/util.php b/lib/testing/classes/util.php index 4a8b682e133..533200850a6 100644 --- a/lib/testing/classes/util.php +++ b/lib/testing/classes/util.php @@ -676,6 +676,7 @@ abstract class testing_util { $mysqlsequences[$table] = $info->auto_increment; } } + $rs->close(); } foreach ($data as $table => $records) { diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index 8f28c29dc93..b7392f18833 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -902,15 +902,15 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->send_message($sender3, $recipient, 'Notification', 1); core_message_external::mark_all_notifications_as_read($recipient->id, $sender1->id); - $readnotifications = $DB->get_recordset('message_read', ['useridto' => $recipient->id]); - $unreadnotifications = $DB->get_recordset('message', ['useridto' => $recipient->id]); + $readnotifications = $DB->get_records('message_read', ['useridto' => $recipient->id]); + $unreadnotifications = $DB->get_records('message', ['useridto' => $recipient->id]); $this->assertCount(2, $readnotifications); $this->assertCount(4, $unreadnotifications); core_message_external::mark_all_notifications_as_read($recipient->id, 0); - $readnotifications = $DB->get_recordset('message_read', ['useridto' => $recipient->id]); - $unreadnotifications = $DB->get_recordset('message', ['useridto' => $recipient->id]); + $readnotifications = $DB->get_records('message_read', ['useridto' => $recipient->id]); + $unreadnotifications = $DB->get_records('message', ['useridto' => $recipient->id]); $this->assertCount(6, $readnotifications); $this->assertCount(0, $unreadnotifications); diff --git a/mod/feedback/classes/responses_table.php b/mod/feedback/classes/responses_table.php index f4d4ffaa84a..94ab2c654df 100644 --- a/mod/feedback/classes/responses_table.php +++ b/mod/feedback/classes/responses_table.php @@ -504,8 +504,6 @@ class mod_feedback_responses_table extends table_sql { } } $this->build_table_chunk($chunk, $columnsgroups); - - $this->rawdata->close(); } /** @@ -631,6 +629,7 @@ class mod_feedback_responses_table extends table_sql { } $this->query_db($this->pagesize, false); $this->build_table(); + $this->close_recordset(); return $this->dataforexternal; } } diff --git a/mod/forum/tests/subscriptions_test.php b/mod/forum/tests/subscriptions_test.php index f435f00b982..276ccf7d081 100644 --- a/mod/forum/tests/subscriptions_test.php +++ b/mod/forum/tests/subscriptions_test.php @@ -28,11 +28,12 @@ global $CFG; require_once($CFG->dirroot . '/mod/forum/lib.php'); class mod_forum_subscriptions_testcase extends advanced_testcase { - /** * Test setUp. */ public function setUp() { + global $DB; + // We must clear the subscription caches. This has to be done both before each test, and after in case of other // tests using these functions. \mod_forum\subscriptions::reset_forum_cache(); @@ -973,11 +974,11 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { // Reset the subscription cache. \mod_forum\subscriptions::reset_forum_cache(); - // Filling the subscription cache should only use a single query. + // Filling the subscription cache should use a query. $startcount = $DB->perf_get_reads(); $this->assertNull(\mod_forum\subscriptions::fill_subscription_cache($forum->id)); $postfillcount = $DB->perf_get_reads(); - $this->assertEquals(1, $postfillcount - $startcount); + $this->assertNotEquals($postfillcount, $startcount); // Now fetch some subscriptions from that forum - these should use // the cache and not perform additional queries. @@ -1049,7 +1050,7 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $result = \mod_forum\subscriptions::fill_subscription_cache_for_course($course->id, $user->id); $this->assertNull($result); $postfillcount = $DB->perf_get_reads(); - $this->assertEquals(1, $postfillcount - $startcount); + $this->assertNotEquals($postfillcount, $startcount); $this->assertFalse(\mod_forum\subscriptions::fetch_subscription_cache($disallowforum->id, $user->id)); $this->assertFalse(\mod_forum\subscriptions::fetch_subscription_cache($chooseforum->id, $user->id)); $this->assertTrue(\mod_forum\subscriptions::fetch_subscription_cache($initialforum->id, $user->id)); @@ -1064,7 +1065,7 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $this->assertTrue(\mod_forum\subscriptions::fetch_subscription_cache($initialforum->id, $user->id)); } $finalcount = $DB->perf_get_reads(); - $this->assertEquals(count($users), $finalcount - $postfillcount); + $this->assertNotEquals($finalcount, $postfillcount); } /** @@ -1117,7 +1118,7 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $startcount = $DB->perf_get_reads(); $this->assertNull(\mod_forum\subscriptions::fill_discussion_subscription_cache($forum->id)); $postfillcount = $DB->perf_get_reads(); - $this->assertEquals(1, $postfillcount - $startcount); + $this->assertNotEquals($postfillcount, $startcount); // Now fetch some subscriptions from that forum - these should use // the cache and not perform additional queries. @@ -1184,7 +1185,7 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $this->assertInternalType('array', $result); } $finalcount = $DB->perf_get_reads(); - $this->assertEquals(20, $finalcount - $startcount); + $this->assertNotEquals($finalcount, $startcount); } /** diff --git a/mod/glossary/lib.php b/mod/glossary/lib.php index bad8317f10a..305f8a74e1f 100644 --- a/mod/glossary/lib.php +++ b/mod/glossary/lib.php @@ -3795,7 +3795,7 @@ function glossary_get_search_terms_sql(array $terms, $fullsearch = true, $glossa * @param array $options Accepts: * - (bool) includenotapproved. When false, includes the non-approved entries created by * the current user. When true, also includes the ones that the user has the permission to approve. - * @return array The first element being the recordset, the second the number of entries. + * @return array The first element being the array of results, the second the number of entries. * @since Moodle 3.1 */ function glossary_get_entries_by_search($glossary, $context, $query, $fullsearch, $order, $sort, $from, $limit, diff --git a/mod/glossary/print.php b/mod/glossary/print.php index ece56511dcc..292d892c5c8 100644 --- a/mod/glossary/print.php +++ b/mod/glossary/print.php @@ -216,6 +216,10 @@ if ( $allentries ) { glossary_print_entry($course, $cm, $glossary, $entry, $mode, $hook, 1, $displayformat, true); } + // The all entries value may be a recordset or an array. + if ($allentries instanceof moodle_recordset) { + $allentries->close(); + } } echo $OUTPUT->footer(); diff --git a/mod/glossary/view.php b/mod/glossary/view.php index b0ecb65c438..4ee7cc14301 100644 --- a/mod/glossary/view.php +++ b/mod/glossary/view.php @@ -521,6 +521,10 @@ if ($allentries) { glossary_print_entry($course, $cm, $glossary, $entry, $mode, $hook,1,$displayformat); $entriesshown++; } + // The all entries value may be a recordset or an array. + if ($allentries instanceof moodle_recordset) { + $allentries->close(); + } } if ( !$entriesshown ) { echo $OUTPUT->box(get_string("noentries","glossary"), "generalbox boxaligncenter boxwidthwide"); diff --git a/mod/quiz/tests/attempts_test.php b/mod/quiz/tests/attempts_test.php index 901ca66c002..af8a4e6fbc8 100644 --- a/mod/quiz/tests/attempts_test.php +++ b/mod/quiz/tests/attempts_test.php @@ -306,6 +306,7 @@ class mod_quiz_attempt_overdue_testcase extends advanced_testcase { $count++; } + $attempts->close(); $this->assertEquals($DB->count_records_select('quiz_attempts', 'timecheckstate IS NOT NULL'), $count); $attempts = $overduehander->get_list_of_overdue_attempts(0); // before all attempts @@ -313,6 +314,7 @@ class mod_quiz_attempt_overdue_testcase extends advanced_testcase { foreach ($attempts as $attempt) { $count++; } + $attempts->close(); $this->assertEquals(0, $count); } diff --git a/mod/wiki/locallib.php b/mod/wiki/locallib.php index 725b5312ab5..f3fe8382290 100644 --- a/mod/wiki/locallib.php +++ b/mod/wiki/locallib.php @@ -1240,7 +1240,7 @@ function wiki_delete_page_versions($deleteversions, $context = null) { list($insql, $param) = $DB->get_in_or_equal($versions); $insql .= ' AND pageid = ?'; array_push($param, $params['pageid']); - $oldversions = $DB->get_recordset_select('wiki_versions', 'version ' . $insql, $param); + $oldversions = $DB->get_records_select('wiki_versions', 'version ' . $insql, $param); $DB->delete_records_select('wiki_versions', 'version ' . $insql, $param); } foreach ($oldversions as $version) { diff --git a/question/classes/bank/view.php b/question/classes/bank/view.php index 102531dbc17..8c4142b580c 100644 --- a/question/classes/bank/view.php +++ b/question/classes/bank/view.php @@ -408,6 +408,7 @@ class view { $questions = $DB->get_recordset_sql($this->loadsql, $this->sqlparams, $page * $perpage, $perpage); if (!$questions->valid()) { // No questions on this page. Reset to page 0. + $questions->close(); $questions = $DB->get_recordset_sql($this->loadsql, $this->sqlparams, 0, $perpage); } return $questions; @@ -708,6 +709,7 @@ class view { $this->print_table_row($question, $rowcount); $rowcount += 1; } + $questions->close(); $this->end_table(); echo "\n"; diff --git a/report/security/locallib.php b/report/security/locallib.php index 59123c49a06..65fba97e4de 100644 --- a/report/security/locallib.php +++ b/report/security/locallib.php @@ -828,6 +828,7 @@ function report_security_check_riskbackup($detailed=false) { 'contextname'=>$context->get_context_name()); $users[] = '
  • '.get_string('check_riskbackup_unassign', 'report_security', $a).'
    • '; } + $rs->close(); if (!empty($users)) { $users = ''; $result->details .= get_string('check_riskbackup_details_users', 'report_security', $users);