mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-01-19 06:18:28 +01:00
Code Issues Projects Releases Wiki Activity

MDL-78580 report_statistics: Prevent database deadlocks when viewing

Browse Source 
If a quiz had a long job to calculate statstics running, this would
cause pages that may also attempt a recalculation (the statistics report
page or question bank) to load very slowly, and possibly result in a
database deadlock.

This change will firstly prevent the question bank page performing
analysis calculations at all, since these are not required for this
page, which will speed up loading and prevent deadlocks on this page.

Secondly, this adds a lock to the recalcuation process so that it cannot
run twice concurrently. This will present the user with a message to
indicate that it is waiting for a running calculation until it is
complete, and eventually it will timeout with a message and debugging.
This commit is contained in:
Mark Johnson 2023-09-13 09:31:32 +01:00
parent b4c6ed3650
commit a92e5c5770
8 changed files with 413 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

76
lib/classes/lock/lock_utils.php Normal file
View File

@ -0,0 +1,76 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
namespace core\lock;
use core\progress\base;
/**
* Lock utilities.
*
* @package core
* @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net}
* @author Mark Johnson <mark.johnson@catalyst-eu.net>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class lock_utils {
/**
* Start a progress bar and attempt to get a lock, updating the bar until a lock is achieved.
*
* This will make multiple attempts at getting the lock using a short timeout set by $progressupdatetime. After
* each failed attempt, it will update the progress bar and try again, until $timeout is reached.
*
* @param lock_factory $lockfactory The factory to use to get the lock
* @param string $resource The resource key we will try to get a lock on
* @param base $progress The progress bar
* @param int $timeout The maximum time in seconds to wait for a lock
* @param string $message Optional message to display on the progress bar
* @param int $progressupdatetime The number of seconds to wait for each lock attempt before updating the progress bar.
* @param int $maxlifetime The maxlifetime to set on the lock, if supported.
* @return lock|false A lock if successful, or false if the timeout expires.
* @throws \coding_exception
*/
public static function wait_for_lock_with_progress(
lock_factory $lockfactory,
string $resource,
\core\progress\base $progress,
int $timeout,
string $message = '',
int $progressupdatetime = 10,
int $maxlifetime = DAYSECS,
) {
if ($progressupdatetime < 1) {
throw new \invalid_parameter_exception('Progress bar cannot update more than once per second. ' .
'$progressupdate time must be at least 1.');
}
if ($progressupdatetime > $timeout) {
throw new \invalid_parameter_exception('$timeout must be greater than $progressupdatetime.');
}
$lockattempts = 0;
$maxattempts = $timeout / $progressupdatetime;
$lock = false;
$progress->start_progress($message);
while (!$lock && $lockattempts < $maxattempts) {
$lock = $lockfactory->get_lock($resource, $progressupdatetime, $maxlifetime);
if (!$lock) {
$progress->progress();
$lockattempts++;
}
}
$progress->end_progress();
return $lock;
}
}

2
mod/quiz/lib.php
View File

@ -2597,5 +2597,5 @@ function mod_quiz_calculate_question_stats(context $context): ?all_calculated_fo
require_once($CFG->dirroot . '/mod/quiz/report/statistics/report.php');
$cm = get_coursemodule_from_id('quiz', $context->instanceid);
$report = new quiz_statistics_report();
return $report->calculate_questions_stats_for_question_bank($cm->instance, false);
return $report->calculate_questions_stats_for_question_bank($cm->instance, false, false);
}

5
mod/quiz/report/statistics/lang/en/quiz_statistics.php
View File

@ -62,6 +62,10 @@ $string['firstattempts'] = 'first attempts';
$string['firstattemptsavg'] = 'Average grade of first attempts';
$string['firstattemptscount'] = 'Number of complete graded first attempts';
$string['frequency'] = 'Frequency';
$string['getstatslockprogress'] = 'Waiting for task in progress. Please wait or try again later.';
$string['getstatslocktimeout'] = 'Statistics calculation lock timeout';
$string['getstatslocktimeoutdesc'] = 'How many seconds to wait for a lock when attempting to perform a statistics calculation ' .
'for a quiz. This setting primarily exists for testing, do not modify it unless you know what you are doing.';
$string['highestattempts'] = 'highest graded attempt';
$string['highestattemptsavg'] = 'Average grade of highest graded attempts';
$string['intended_weight'] = 'Intended weight';
@ -83,6 +87,7 @@ Our equation for effective question weight cannot be calculated in this case. Th
If you edit a quiz and give these question(s) with negative covariance a max grade of zero then the effective question weight of these questions will be zero and the real effective question weight of other questions will be as calculated now.';
$string['nogradedattempts'] = 'No attempts have been made at this quiz, or all attempts have questions that need manual grading.';
$string['nostudentsingroup'] = 'There are no students in this group yet';
$string['nostats'] = 'Could not complete the statistics calculation. There may be a long-running calculation in progress. Please try again later.';
$string['optiongrade'] = 'Partial credit';
$string['partofquestion'] = 'Part of question';
$string['pluginname'] = 'Statistics';

83
mod/quiz/report/statistics/report.php
View File

@ -157,6 +157,10 @@ class quiz_statistics_report extends report_base {
$progress = $this->get_progress_trace_instance();
list($quizstats, $questionstats) =
$this->get_all_stats_and_analysis($quiz, $whichattempts, $whichtries, $groupstudentsjoins, $questions, $progress);
if (is_null($quizstats)) {
echo $OUTPUT->notification(get_string('nostats', 'quiz_statistics'), 'error');
return true;
}
} else {
// Or create empty stats containers.
$quizstats = new \quiz_statistics\calculated($whichattempts);
@ -624,13 +628,15 @@ class quiz_statistics_report extends report_base {
* @param \core\progress\base|null $progress
* @param bool $calculateifrequired if true (the default) the stats will be calculated if not already stored.
* If false, [null, null] will be returned if the stats are not already available.
* @param bool $performanalysis if true (the default) and there are calculated stats, analysis will be performed
* for each question.
* @return array with 2 elements: - $quizstats The statistics for overall attempt scores.
* - $questionstats \core_question\statistics\questions\all_calculated_for_qubaid_condition
* Both may be null, if $calculateifrequired is false.
*/
public function get_all_stats_and_analysis(
$quiz, $whichattempts, $whichtries, \core\dml\sql_join $groupstudentsjoins,
$questions, $progress = null, bool $calculateifrequired = true) {
$questions, $progress = null, bool $calculateifrequired = true, bool $performanalysis = true) {
if ($progress === null) {
$progress = new \core\progress\none();
@ -642,38 +648,82 @@ class quiz_statistics_report extends report_base {
$quizcalc = new \quiz_statistics\calculator($progress);
$progress->start_progress('', 3);
$progress->start_progress('', 4);
// Get a lock on this set of qubaids before performing calculations. This prevents the same calculation running
// concurrently and causing database deadlocks. We use a long timeout here as a big quiz with lots of attempts may
// take a long time to process.
$lockfactory = \core\lock\lock_config::get_lock_factory('quiz_statistics_get_stats');
$lock = $lockfactory->get_lock($qubaids->get_hash_code(), 0);
if (!$lock) {
if (!$calculateifrequired) {
// We're not going to do the calculation in this request anyway, so just give up here.
$progress->progress(4);
$progress->end_progress();
return [null, null];
}
$locktimeout = get_config('quiz_statistics', 'getstatslocktimeout');
$lock = \core\lock\lock_utils::wait_for_lock_with_progress(
$lockfactory,
$qubaids->get_hash_code(),
$progress,
$locktimeout,
get_string('getstatslockprogress', 'quiz_statistics'),
);
if (!$lock) {
// Lock attempt timed out.
$progress->progress(4);
$progress->end_progress();
debugging('Could not get lock on ' .
$qubaids->get_hash_code() . ' (Quiz ID ' . $quiz->id . ') after ' .
$locktimeout . ' seconds');
return [null, null];
}
}
try {
if ($quizcalc->get_last_calculated_time($qubaids) === false) {
if (!$calculateifrequired) {
$progress->progress(3);
$progress->progress(4);
$progress->end_progress();
$lock->release();
return [null, null];
}
// Recalculate now.
$questionstats = $qcalc->calculate($qubaids);
$progress->progress(1);
$quizstats = $quizcalc->calculate($quiz->id, $whichattempts, $groupstudentsjoins, count($questions),
$qcalc->get_sum_of_mark_variance());
$progress->progress(2);
$quizstats = $quizcalc->calculate(
$quiz->id,
$whichattempts,
$groupstudentsjoins,
count($questions),
$qcalc->get_sum_of_mark_variance()
);
$progress->progress(3);
} else {
$quizstats = $quizcalc->get_cached($qubaids);
$progress->progress(1);
$questionstats = $qcalc->get_cached($qubaids);
$progress->progress(2);
$questionstats = $qcalc->get_cached($qubaids);
$progress->progress(3);
}
if ($quizstats->s()) {
if ($quizstats->s() && $performanalysis) {
$subquestions = $questionstats->get_sub_questions();
$this->analyse_responses_for_all_questions_and_subquestions($questions,
$this->analyse_responses_for_all_questions_and_subquestions(
$questions,
$subquestions,
$qubaids,
$whichtries,
$progress);
$progress
);
}
$progress->progress(3);
$progress->progress(4);
$progress->end_progress();
} finally {
$lock->release();
}
return [$quizstats, $questionstats];
}
@ -932,11 +982,14 @@ class quiz_statistics_report extends report_base {
* @param int $quizid question usage
* @param bool $calculateifrequired if true (the default) the stats will be calculated if not already stored.
* If false, null will be returned if the stats are not already available.
* @param bool $performanalysis if true (the default) and there are calculated stats, analysis will be performed
* for each question.
* @return ?all_calculated_for_qubaid_condition question stats
*/
public function calculate_questions_stats_for_question_bank(
int $quizid,
bool $calculateifrequired = true
bool $calculateifrequired = true,
bool $performanalysis = true,
): ?all_calculated_for_qubaid_condition {
global $DB;
$quiz = $DB->get_record('quiz', ['id' => $quizid], '*', MUST_EXIST);
@ -944,7 +997,7 @@ class quiz_statistics_report extends report_base {
[, $questionstats] = $this->get_all_stats_and_analysis($quiz,
$quiz->grademethod, question_attempt::ALL_TRIES, new \core\dml\sql_join(),
$questions, null, $calculateifrequired);
$questions, null, $calculateifrequired, $performanalysis);
return $questionstats;
}

35
mod/quiz/report/statistics/settings.php Normal file
View File

@ -0,0 +1,35 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
/**
* Settings for the Statisics report
*
* @package quiz_statistics
* @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net}
* @author Mark Johnson <mark.johnson@catalyst-eu.net>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
defined('MOODLE_INTERNAL') || die;
if ($ADMIN->fulltree) {
$settings->add(new admin_setting_configtext(
'quiz_statistics/getstatslocktimeout',
get_string('getstatslocktimeout', 'quiz_statistics'),
get_string('getstatslocktimeoutdesc', 'quiz_statistics'),
MINSECS * 15
));
}

212
mod/quiz/report/statistics/tests/test_quiz_statistics_report.php Normal file
View File

@ -0,0 +1,212 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
namespace quiz_statistics;
defined('MOODLE_INTERNAL') || die();
global $CFG;
require_once($CFG->dirroot . '/mod/quiz/report/statistics/report.php');
require_once($CFG->dirroot . '/mod/quiz/report/statistics/statisticslib.php');
require_once($CFG->dirroot . '/mod/quiz/report/reportlib.php');
require_once($CFG->dirroot . '/mod/quiz/tests/quiz_question_helper_test_trait.php');
/**
* Tests for statistics report
*
* @package quiz_statistics
* @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net}
* @author Mark Johnson <mark.johnson@catalyst-eu.net>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* @covers \quiz_statistics_report
*/
class test_quiz_statistics_report extends \advanced_testcase {
use \quiz_question_helper_test_trait;
/**
* Secondary database connection for creating locks.
*
* @var \moodle_database|null
*/
protected static ?\moodle_database $lockdb;
/**
* Lock factory using the secondary database connection.
*
* @var \moodle_database|null
*/
protected static ?\core\lock\lock_factory $lockfactory;
/**
* Create a lock factory with a second database session.
*
* This allows us to create a lock in our test code that will block a lock request
* on the same key in code under test.
*
* @return void
*/
public static function setUpBeforeClass(): void {
global $CFG;
self::$lockdb = \moodle_database::get_driver_instance($CFG->dbtype, $CFG->dblibrary);
self::$lockdb->connect($CFG->dbhost, $CFG->dbuser, $CFG->dbpass, $CFG->dbname, $CFG->prefix, $CFG->dboptions);
$lockfactory = \core\lock\lock_config::get_lock_factory('quiz_statistics_get_stats');
$reflectiondb = new \ReflectionProperty($lockfactory, 'db');
$reflectiondb->setAccessible(true);
$reflectiondb->setValue($lockfactory, self::$lockdb);
self::$lockfactory = $lockfactory;
}
/**
* Dispose of the extra DB connection and lock factory.
*
* @return void
*/
public static function tearDownAfterClass(): void {
self::$lockdb->dispose();
self::$lockdb = null;
self::$lockfactory = null;
}
/**
* Return a generated quiz
*
* @return \stdClass
*/
protected function create_and_attempt_quiz(): \stdClass {
$course = $this->getDataGenerator()->create_course();
$user = $this->getDataGenerator()->create_user();
$quiz = $this->create_test_quiz($course);
$quizcontext = \context_module::instance($quiz->cmid);
$questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question');
$this->add_two_regular_questions($questiongenerator, $quiz, ['contextid' => $quizcontext->id]);
$this->attempt_quiz($quiz, $user);
return $quiz;
}
/**
* Test locking the calculation process.
*
* When there is a lock on the hash code, test_get_all_stats_and_analysis() should wait until the lock timeout, then throw an
* exception.
*
* When there is no lock (or the lock has been released), it should return a result.
*
* @return void
*/
public function test_get_all_stats_and_analysis_locking(): void {
$this->resetAfterTest(true);
$quiz = $this->create_and_attempt_quiz();
$whichattempts = QUIZ_GRADEAVERAGE; // All attempts.
$whichtries = \question_attempt::ALL_TRIES;
$groupstudentsjoins = new \core\dml\sql_join();
$qubaids = quiz_statistics_qubaids_condition($quiz->id, $groupstudentsjoins, $whichattempts);
$report = new \quiz_statistics_report();
$questions = $report->load_and_initialise_questions_for_calculations($quiz);
$timeoutseconds = 20;
set_config('getstatslocktimeout', $timeoutseconds, 'quiz_statistics');
$lock = self::$lockfactory->get_lock($qubaids->get_hash_code(), 0);
$progress = new \core\progress\none();
$this->resetDebugging();
$timebefore = microtime(true);
try {
$result = $report->get_all_stats_and_analysis(
$quiz,
$whichattempts,
$whichtries,
$groupstudentsjoins,
$questions,
$progress
);
$timeafter = microtime(true);
// Verify that we waited as long as the timeout.
$this->assertEqualsWithDelta($timeoutseconds, $timeafter - $timebefore, 1);
$this->assertDebuggingCalled('Could not get lock on ' .
$qubaids->get_hash_code() . ' (Quiz ID ' . $quiz->id . ') after ' .
$timeoutseconds . ' seconds');
$this->assertEquals([null, null], $result);
} finally {
$lock->release();
}
$this->resetDebugging();
$result = $report->get_all_stats_and_analysis(
$quiz,
$whichattempts,
$whichtries,
$groupstudentsjoins,
$questions
);
$this->assertDebuggingNotCalled();
$this->assertNotEquals([null, null], $result);
}
/**
* Test locking when the current page does not require calculations.
*
* When there is a lock on the hash code, test_get_all_stats_and_analysis() should return a null result immediately,
* with no exception thrown.
*
* @return void
*/
public function test_get_all_stats_and_analysis_locking_no_calculation(): void {
$this->resetAfterTest(true);
$quiz = $this->create_and_attempt_quiz();
$whichattempts = QUIZ_GRADEAVERAGE; // All attempts.
$whichtries = \question_attempt::ALL_TRIES;
$groupstudentsjoins = new \core\dml\sql_join();
$qubaids = quiz_statistics_qubaids_condition($quiz->id, $groupstudentsjoins, $whichattempts);
$report = new \quiz_statistics_report();
$questions = $report->load_and_initialise_questions_for_calculations($quiz);
$timeoutseconds = 20;
set_config('getstatslocktimeout', $timeoutseconds, 'quiz_statistics');
$lock = self::$lockfactory->get_lock($qubaids->get_hash_code(), 0);
$this->resetDebugging();
try {
$progress = new \core\progress\none();
$timebefore = microtime(true);
$result = $report->get_all_stats_and_analysis(
$quiz,
$whichattempts,
$whichtries,
$groupstudentsjoins,
$questions,
$progress,
false
);
$timeafter = microtime(true);
// Verify that we did not wait for the timeout before returning.
$this->assertLessThan($timeoutseconds, $timeafter - $timebefore);
$this->assertEquals([null, null], $result);
$this->assertDebuggingNotCalled();
} finally {
$lock->release();
}
}
}

2
mod/quiz/report/statistics/version.php
View File

@ -24,6 +24,6 @@
defined('MOODLE_INTERNAL') || die();
$plugin->version = 2023042400;
$plugin->version = 2023042402;
$plugin->requires = 2023041800;
$plugin->component = 'quiz_statistics';

3
question/classes/statistics/questions/calculator.php
View File

@ -257,10 +257,9 @@ class calculator {
}
}
$this->stats->cache($qubaids);
}
// All finished.
$this->progress->end_progress();
}
return $this->stats;
}