Skip to content

Commit

Permalink
Merge branch 'MDL-73713-readonly-txn-latency' of https://github.com/c…
Browse files Browse the repository at this point in the history 
…atalyst/moodle
  • Loading branch information
@junpataleta
junpataleta committed Mar 24, 2022
2 parents 7a65b2a + d8baf29 commit efe227d
Show file tree
Hide file tree
Showing 13 changed files with 227 additions and 60 deletions.
5 changes: 3 additions & 2 deletions config-dist.php
View file
Expand Up @@ -105,8 +105,9 @@
'latency' => 0.5, // Set read-only slave sync latency in seconds.
// When 'latency' seconds have lapsed after an update to a table
// it is deemed safe to use readonly slave for reading from the table.
// It is optional. If omitted once written to a table it will always
// use master handle for reading.
// It is optional, defaults to 1 second. If you want once written to a table
// to always use master handle for reading set it to something ridiculosly big,
// eg 10.
// Lower values increase the performance, but setting it too low means
// missing the master-slave sync.
'exclude_tables' => [ // Tables to exclude from read-only slave feature.
Expand Down
60 changes: 27 additions & 33 deletions lib/dml/moodle_read_slave_trait.php
View file
Expand Up @@ -43,8 +43,8 @@
* - It supports multiple 'instance' entries, in case one is not accessible,
* but only one (first connectable) instance is used.
* - 'latency' option: master -> slave sync latency in seconds (will probably
* be a fraction of a second). If specified, a table being written to is
* deemed fully synced and suitable for slave read.
* be a fraction of a second). A table being written to is deemed fully synced
* after that period and suitable for slave read. Defaults to 1 sec.
* - 'exclude_tables' option: a list of tables that never go to the slave for
* querying. The feature is meant to be used in emergency only, so the
* readonly feature can still be used in case there is a rogue query that
Expand All @@ -54,20 +54,18 @@
*
* Choice of the database handle is based on following:
* - SQL_QUERY_INSERT, UPDATE and STRUCTURE record table from the query
* in the $written array and microtime() the event if the 'latency' option
* is set. For those queries master write handle is used.
* in the $written array and microtime() the event. For those queries master
* write handle is used.
* - SQL_QUERY_AUX queries will always use the master write handle because they
* are used for transactionstart/end, locking etc. In that respect, query_start() and
* query_end() *must not* be used during the connection phase.
* - SELECT queries will use the master write handle if:
* -- any of the tables involved is a temp table
* -- any of the tables involved is listed in the 'exclude_tables' option
* -- any of the tables involved is in the $written array:
* * If the 'latency' option is set then the microtime() is compared to
* the write microrime, and if more then latency time has passed the slave
* handle is used.
* * Otherwise (not enough time passed or 'latency' option not set)
* we choose the master write handle
* * current microtime() is compared to the write microrime, and if more than
* latency time has passed the slave handle is used
* * otherwise (not enough time passed) we choose the master write handle
* If none of the above conditions are met the slave instance is used.
*
* A 'latency' example:
Expand All @@ -92,7 +90,7 @@ trait moodle_read_slave_trait {

private $wantreadslave = false;
private $readsslave = 0;
private $slavelatency = 0;
private $slavelatency = 1;

private $written = []; // Track tables being written to.
private $readexclude = []; // Tables to exclude from using dbhreadonly.
Expand Down Expand Up @@ -324,23 +322,27 @@ protected function can_use_readonly(int $type, string $sql): bool {
}

if (isset($this->written[$tablename])) {
if ($this->slavelatency) {
$now = $now ?: microtime(true);
if ($now - $this->written[$tablename] < $this->slavelatency) {
return false;
}
unset($this->written[$tablename]);
} else {
$now = $now ?: microtime(true);
// Paranoid check.
if ($this->written[$tablename] === true) {
debugging(
"$tablename last written set to true outside transaction - should not happen!",
DEBUG_DEVELOPER
);
$this->written[$tablename] = $now;
return false;
}
if ($now - $this->written[$tablename] < $this->slavelatency) {
return false;
}
unset($this->written[$tablename]);
}
}

return true;
case SQL_QUERY_INSERT:
case SQL_QUERY_UPDATE:
// If we are in transaction we cannot set the written time yet.
$now = $this->slavelatency && !$this->transactions ? microtime(true) : true;
$now = $this->transactions ? true : microtime(true);
foreach ($this->table_names($sql) as $tablename) {
$this->written[$tablename] = $now;
}
Expand All @@ -364,23 +366,15 @@ protected function can_use_readonly(int $type, string $sql): bool {
* @throws dml_transaction_exception Creates and throws transaction related exceptions.
*/
public function commit_delegated_transaction(moodle_transaction $transaction) {
parent::commit_delegated_transaction($transaction);

if ($this->transactions) {
return;
}

if (!$this->slavelatency) {
return;
}

$now = null;
foreach ($this->written as $tablename => $when) {
if ($when === true) {
$now = $now ?: microtime(true);
if ($this->written) {
// Adjust the written time.
$now = microtime(true);
foreach ($this->written as $tablename => $when) {
$this->written[$tablename] = $now;
}
}

parent::commit_delegated_transaction($transaction);
}

/**
Expand Down
7 changes: 5 additions & 2 deletions lib/dml/tests/dml_mysqli_read_slave_test.php
View file
Expand Up @@ -21,8 +21,11 @@
* @category dml
* @copyright 2018 Srdjan Janković, Catalyst IT
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* @coversDefaultClass \mysqli_native_moodle_database
*/

namespace core;

defined('MOODLE_INTERNAL') || die();

require_once(__DIR__.'/fixtures/read_slave_moodle_database_mock_mysqli.php');
Expand All @@ -35,7 +38,7 @@
* @copyright 2018 Catalyst IT
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class core_dml_mysqli_read_slave_testcase extends base_testcase {
class dml_mysqli_read_slave_test extends \base_testcase {
/**
* Test readonly handle is not used for reading from special pg_*() call queries,
* pg_try_advisory_lock and pg_advisory_unlock.
Expand Down Expand Up @@ -80,7 +83,7 @@ public function test_real_readslave_connect_fail() : void {
'connecttimeout' => 1
];

$db2 = moodle_database::get_driver_instance($cfg->dbtype, $cfg->dblibrary);
$db2 = \moodle_database::get_driver_instance($cfg->dbtype, $cfg->dblibrary);
$db2->connect($cfg->dbhost, $cfg->dbuser, $cfg->dbpass, $cfg->dbname, $cfg->prefix, $cfg->dboptions);
$this->assertTrue(count($db2->get_records('user')) > 0);
}
Expand Down
13 changes: 8 additions & 5 deletions lib/dml/tests/dml_pgsql_read_slave_test.php
View file
Expand Up @@ -21,8 +21,11 @@
* @category dml
* @copyright 2018 Srdjan Janković, Catalyst IT
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* @coversDefaultClass \pgsql_native_moodle_database
*/

namespace core;

defined('MOODLE_INTERNAL') || die();

require_once(__DIR__.'/fixtures/read_slave_moodle_database_mock_pgsql.php');
Expand All @@ -35,7 +38,7 @@
* @copyright 2018 Catalyst IT
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class core_dml_pgsql_read_slave_testcase extends base_testcase {
class dml_pgsql_read_slave_test extends \base_testcase {
/**
* Test correct database handles are used for cursors
*
Expand Down Expand Up @@ -135,12 +138,12 @@ public function test_temp_table() : void {
}

// Get a separate disposable db connection handle with guaranteed 'readonly' config.
$db2 = moodle_database::get_driver_instance($cfg->dbtype, $cfg->dblibrary);
$db2 = \moodle_database::get_driver_instance($cfg->dbtype, $cfg->dblibrary);
$db2->connect($cfg->dbhost, $cfg->dbuser, $cfg->dbpass, $cfg->dbname, $cfg->prefix, $cfg->dboptions);

$dbman = $db2->get_manager();

$table = new xmldb_table('silly_test_table');
$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']);
Expand All @@ -155,7 +158,7 @@ public function test_temp_table() : void {
$db2->get_records('silly_test_table');
$this->assertEquals($reads, $db2->perf_get_reads_slave());

$table2 = new xmldb_table('silly_test_table2');
$table2 = new \xmldb_table('silly_test_table2');
$table2->add_field('id', XMLDB_TYPE_INTEGER, 10, null, XMLDB_NOTNULL, XMLDB_SEQUENCE);
$table2->add_field('msg', XMLDB_TYPE_CHAR, 255);
$table2->add_key('primary', XMLDB_KEY_PRIMARY, ['id']);
Expand Down Expand Up @@ -193,7 +196,7 @@ public function test_real_readslave_connect_fail() : void {
'connecttimeout' => 1
];

$db2 = moodle_database::get_driver_instance($cfg->dbtype, $cfg->dblibrary);
$db2 = \moodle_database::get_driver_instance($cfg->dbtype, $cfg->dblibrary);
$db2->connect($cfg->dbhost, $cfg->dbuser, $cfg->dbpass, $cfg->dbname, $cfg->prefix, $cfg->dboptions);
$this->assertTrue(count($db2->get_records('user')) > 0);
}
Expand Down
111 changes: 105 additions & 6 deletions lib/dml/tests/dml_read_slave_test.php
View file
Expand Up @@ -21,12 +21,16 @@
* @category dml
* @copyright 2018 Srdjan Janković, Catalyst IT
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* @coversDefaultClass \moodle_temptables
*/

namespace core;

defined('MOODLE_INTERNAL') || die();

require_once(__DIR__.'/fixtures/read_slave_moodle_database_table_names.php');
require_once(__DIR__.'/fixtures/read_slave_moodle_database_special.php');
require_once(__DIR__.'/../../tests/fixtures/event_fixtures.php');

/**
* DML read/read-write database handle use tests
Expand All @@ -36,10 +40,12 @@
* @copyright 2018 Catalyst IT
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class core_dml_read_slave_testcase extends base_testcase {
class dml_read_slave_test extends \base_testcase {

/** @var float */
static private $dbreadonlylatency = 0.8;
/** @var float */
static private $defaultlatency = 1;

/**
* Instantiates a test database interface object.
Expand Down Expand Up @@ -223,18 +229,27 @@ public function test_write_read_read() : void {
$this->assertEquals('test_rw::test:test', $handle);
$this->assertEquals(0, $DB->perf_get_reads_slave());

sleep(1);
$handle = $DB->get_records('table');
$this->assertEquals('test_rw::test:test', $handle);
$this->assertEquals(0, $DB->perf_get_reads_slave());

$handle = $DB->get_records('table2');
$handle = $DB->get_records_sql("SELECT * FROM {table2} JOIN {table}");
$this->assertEquals('test_rw::test:test', $handle);
$this->assertEquals(0, $DB->perf_get_reads_slave());

sleep(1);

$handle = $DB->get_records('table');
$this->assert_readonly_handle($handle);
$this->assertEquals(1, $DB->perf_get_reads_slave());

$handle = $DB->get_records('table2');
$this->assert_readonly_handle($handle);
$this->assertEquals(2, $DB->perf_get_reads_slave());

$handle = $DB->get_records_sql("SELECT * FROM {table2} JOIN {table}");
$this->assertEquals('test_rw::test:test', $handle);
$this->assertEquals(1, $DB->perf_get_reads_slave());
$this->assert_readonly_handle($handle);
$this->assertEquals(3, $DB->perf_get_reads_slave());
}

/**
Expand Down Expand Up @@ -278,12 +293,15 @@ public function test_read_excluded_tables() : void {
* so the latency parameter is applied properly.
*
* @return void
* @covers ::can_use_readonly
* @covers ::commit_delegated_transaction
*/
public function test_transaction() : void {
public function test_transaction(): void {
$DB = $this->new_db(true);

$this->assertNull($DB->get_dbhwrite());

$skip = false;
$transaction = $DB->start_delegated_transaction();
$now = microtime(true);
$handle = $DB->get_records_sql("SELECT * FROM {table}");
Expand All @@ -304,11 +322,92 @@ public function test_transaction() : void {

// Make sure enough time passes.
sleep(1);
} else {
$skip = true;
}

// Exceeded latency time, use ro handle.
$handle = $DB->get_records_sql("SELECT * FROM {table}");
$this->assert_readonly_handle($handle);

if ($skip) {
$this->markTestSkipped("Delay too long to test write handle immediately after transaction");
}
}

/**
* Test readonly handle is not used with events
* when the latency parameter is applied properly.
*
* @return void
* @covers ::can_use_readonly
* @covers ::commit_delegated_transaction
*/
public function test_transaction_with_events(): void {
$this->with_global_db(function () {
global $DB;

$DB = $this->new_db(true, ['test_ro'], read_slave_moodle_database_special::class);
$DB->set_tables([
'config_plugins' => [
'columns' => [
'plugin' => (object)['meta_type' => ''],
]
]
]);

$this->assertNull($DB->get_dbhwrite());

$this->_called = false;
$transaction = $DB->start_delegated_transaction();
$now = microtime(true);

$observers = [
[
'eventname' => '\core_tests\event\unittest_executed',
'callback' => function (\core_tests\event\unittest_executed $event) use ($DB, $now) {
$this->_called = true;
$this->assertFalse($DB->is_transaction_started());

// This condition should always evaluate true, however we need to
// safeguard from an unaccounted delay that can break this test.
if (microtime(true) - $now < 1 + self::$dbreadonlylatency) {
// Not enough time passed, use rw handle.
$handle = $DB->get_records_sql_p("SELECT * FROM {table}");
$this->assertEquals('test_rw::test:test', $handle);

// Make sure enough time passes.
sleep(1);
} else {
$this->markTestSkipped("Delay too long to test write handle immediately after transaction");
}

// Exceeded latency time, use ro handle.
$handle = $DB->get_records_sql_p("SELECT * FROM {table}");
$this->assertEquals('test_ro::test:test', $handle);
},
'internal' => 0,
],
];
\core\event\manager::phpunit_replace_observers($observers);

$handle = $DB->get_records_sql_p("SELECT * FROM {table}");
// Use rw handle during transaction.
$this->assertEquals('test_rw::test:test', $handle);

$handle = $DB->insert_record_raw('table', array('name' => 'blah'));
// Introduce delay so we can check that table write timestamps
// are adjusted properly.
sleep(1);
$event = \core_tests\event\unittest_executed::create([
'context' => \context_system::instance(),
'other' => ['sample' => 1]
]);
$event->trigger();
$transaction->allow_commit();

$this->assertTrue($this->_called);
});
}

/**
Expand Down

0 comments on commit efe227d

Please sign in to comment.