From 838f07703afc5cc04dbde10b3b6c40b39371ceab Mon Sep 17 00:00:00 2001 From: sam marshall Date: Tue, 10 Oct 2017 15:08:54 +0100 Subject: [PATCH 1/4] MDL-60915 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 a85023dc054..ae7550175e4 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() @@ -736,14 +742,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; } /** @@ -1368,7 +1449,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); @@ -1383,7 +1464,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); @@ -1398,7 +1479,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 f83d748f7767f1621ed2aebbffe366dacc6eaed5 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Wed, 11 Oct 2017 12:16:12 +0100 Subject: [PATCH 2/4] MDL-60915 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 9b87054f72e..529645f9bf8 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 c319946dc7a..f3c3a16a933 100644 --- a/cohort/lib.php +++ b/cohort/lib.php @@ -507,6 +507,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 99a4c6ec309..57bd61610da 100644 --- a/group/lib.php +++ b/group/lib.php @@ -628,6 +628,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)); @@ -658,6 +659,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)); @@ -690,6 +692,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 7e2d79b6707..c8fa2455a08 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 a731b661628..376a3010071 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 e15d0715d17..2c1906db990 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 11ef46b7200..226d024c199 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 9426775eba1..50b7d9efbff 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 7edc45b777b..c78f9a2a5f1 100644 --- a/mod/glossary/lib.php +++ b/mod/glossary/lib.php @@ -3805,7 +3805,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 b24ca88b3b3..5adc32fc268 100644 --- a/mod/glossary/view.php +++ b/mod/glossary/view.php @@ -524,6 +524,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 a0f1dae918b..00a638d8ef4 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 a7e67401868..acbd1483133 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); From 9d97d1e0069c1156ffd77739e501cf24ed450524 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Tue, 28 Nov 2017 13:21:49 +0000 Subject: [PATCH 3/4] MDL-60915 Document new dboption in config-dist.php --- config-dist.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/config-dist.php b/config-dist.php index 6c60e81b690..5308052c915 100644 --- a/config-dist.php +++ b/config-dist.php @@ -70,6 +70,15 @@ $CFG->dboptions = array( // can be removed for MySQL (by default it will // use 'utf8mb4_unicode_ci'. This option should // be removed for all other databases. + // 'fetchbuffersize' => 100000, // On PostgreSQL, this option sets a limit + // on the number of rows that are fetched into + // memory when doing a large recordset query + // (e.g. search indexing). Default is 100000. + // Uncomment and set to a value to change it, + // or zero to turn off the limit. You need to + // set to zero if you are using pg_bouncer in + // 'transaction' mode (it is fine in 'session' + // mode). ); From 7cba6c16710a98061aa0ff6e771ee4be6a96bdc7 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Mon, 2 Jul 2018 01:15:49 +0200 Subject: [PATCH 4/4] MDL-60915 core_dml: Change behavior to disabled by default In stables it was decided to keep the current behavior (not buffering/cursors) so nothing breaks/changes suddenly. So this commit just disables the default buffer size used in Moodle 3.5 and up (100000), defaulting to zero (feature disabled). Still the feature can be enabled via dboptions and it's docummented in the config-dist file. --- config-dist.php | 15 +++++++++------ lib/dml/pgsql_native_moodle_database.php | 5 +---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/config-dist.php b/config-dist.php index 5308052c915..0803045c7a7 100644 --- a/config-dist.php +++ b/config-dist.php @@ -73,12 +73,15 @@ $CFG->dboptions = array( // 'fetchbuffersize' => 100000, // On PostgreSQL, this option sets a limit // on the number of rows that are fetched into // memory when doing a large recordset query - // (e.g. search indexing). Default is 100000. - // Uncomment and set to a value to change it, - // or zero to turn off the limit. You need to - // set to zero if you are using pg_bouncer in - // 'transaction' mode (it is fine in 'session' - // mode). + // (e.g. search indexing). + // By default, this feature is disabled in + // Moodle 3.4, using a value of zero. In Moodle + // 3.5 and up the feature is enabled by default + // with a buffer size of 100000. + // Uncomment and set a positive value to enable it, + // noting that you need to keep it to zero + // if you are using pg_bouncer in 'transaction' + // mode (it is fine in 'session' mode). ); diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php index ae7550175e4..7e87010556e 100644 --- a/lib/dml/pgsql_native_moodle_database.php +++ b/lib/dml/pgsql_native_moodle_database.php @@ -48,9 +48,6 @@ class pgsql_native_moodle_database extends moodle_database { /** @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() @@ -782,7 +779,7 @@ class pgsql_native_moodle_database extends moodle_database { if (array_key_exists('fetchbuffersize', $this->dboptions)) { return (int)$this->dboptions['fetchbuffersize']; } else { - return self::DEFAULT_FETCH_BUFFER_SIZE; + return 0; // Disabled by default. } }