From fa0eecd6bfda149b05675415fce846250c42ae91 Mon Sep 17 00:00:00 2001
From: Srdjan <srdjan@catalyst.net.nz>
Date: Thu, 6 Aug 2020 14:35:27 +1000
Subject: [PATCH] MDL-69451 dml: use same temptables for both rw and ro
 database handle

moodle_read_slave_trait: when creating another handle, restore temptables
property that is clobbered by raw_connect().

Also a better condition for temptable related queries detection in
pgsql_native_moodle_database.

dml_pgsql_read_slave_test::test_temp_table(): use real db connection
if possible, otherwise skip the test.
---
 lib/dml/moodle_read_slave_trait.php         |  6 ++-
 lib/dml/pgsql_native_moodle_database.php    |  2 +-
 lib/dml/tests/dml_pgsql_read_slave_test.php | 51 +++++++++++++++++----
 3 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/lib/dml/moodle_read_slave_trait.php b/lib/dml/moodle_read_slave_trait.php
index 95d05a49845..17861511b32 100644
--- a/lib/dml/moodle_read_slave_trait.php
+++ b/lib/dml/moodle_read_slave_trait.php
@@ -226,9 +226,13 @@ trait moodle_read_slave_trait {
      * @return void
      */
     private function set_dbhwrite(): void {
-        // Late connect to read/write master if needed.
+        // Lazy connect to read/write master.
         if (!$this->dbhwrite) {
+            $temptables = $this->temptables;
             $this->raw_connect($this->pdbhost, $this->pdbuser, $this->pdbpass, $this->pdbname, $this->pprefix, $this->pdboptions);
+            if ($temptables) {
+                $this->temptables = $temptables; // Restore temptables, so we don't get separate sets for rw and ro.
+            }
             $this->dbhwrite = $this->get_db_handle();
         }
         $this->set_db_handle($this->dbhwrite);
diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php
index 88c3261b907..93903984bcf 100644
--- a/lib/dml/pgsql_native_moodle_database.php
+++ b/lib/dml/pgsql_native_moodle_database.php
@@ -295,7 +295,7 @@ class pgsql_native_moodle_database extends moodle_database {
         }
 
         // ... a nuisance - temptables use this.
-        if (preg_match('/\bpg_constraint/', $sql) && $this->temptables->get_temptables()) {
+        if (preg_match('/\bpg_catalog/', $sql) && $this->temptables->get_temptables()) {
             return false;
         }
 
diff --git a/lib/dml/tests/dml_pgsql_read_slave_test.php b/lib/dml/tests/dml_pgsql_read_slave_test.php
index 354057e6b7f..b1f24ebbf9f 100644
--- a/lib/dml/tests/dml_pgsql_read_slave_test.php
+++ b/lib/dml/tests/dml_pgsql_read_slave_test.php
@@ -117,25 +117,58 @@ class core_dml_pgsql_read_slave_testcase extends base_testcase {
      * @return void
      */
     public function test_temp_table() : void {
-        if (PHP_VERSION_ID >= 80000) {
-            // TODO MDL-71482 - there seems to be a bigger problem here than just failing test on PHP8.
-            $this->markTestSkipped();
+        global $DB;
+
+        if ($DB->get_dbfamily() != 'postgres') {
+            $this->markTestSkipped("Not postgres");
         }
 
-        $DB = new read_slave_moodle_database_mock_pgsql();
+        // Open second connection.
+        $cfg = $DB->export_dbconfig();
+        if (!isset($cfg->dboptions)) {
+            $cfg->dboptions = [];
+        }
+        if (!isset($cfg->dboptions['readonly'])) {
+            $cfg->dboptions['readonly'] = [
+                'instance' => [$cfg->dbhost]
+            ];
+        }
 
-        $this->assertEquals(0, $DB->perf_get_reads_slave());
+        // Get a separate disposable db connection handle with guaranteed 'readonly' config.
+        $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();
 
-        $dbman = $DB->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);
 
-        $DB->get_columns('silly_test_table');
-        $DB->get_records('silly_test_table');
-        $this->assertEquals(0, $DB->perf_get_reads_slave());
+        // We need to go through the creation proces twice.
+        // create_temp_table() performs some reads before the temp table is created.
+        // First time around those reads should go to ro ...
+        $reads = $db2->perf_get_reads_slave();
+
+        $db2->get_columns('silly_test_table');
+        $db2->get_records('silly_test_table');
+        $this->assertEquals($reads, $db2->perf_get_reads_slave());
+
+        $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']);
+        $dbman->create_temp_table($table2);
+
+        // ... but once the first temp table is created no more ro reads should occur.
+        $db2->get_columns('silly_test_table2');
+        $db2->get_records('silly_test_table2');
+        $this->assertEquals($reads, $db2->perf_get_reads_slave());
+
+        // Make database driver happy.
+        $dbman->drop_table($table2);
+        $dbman->drop_table($table);
     }
 
     /**