-
Bug
-
Resolution: Deferred
-
Major
-
None
-
3.10.3
-
None
-
MOODLE_310_STABLE
As dobedobedoh commented here:
The issue that has caused you to remove the test is that pg_query and pg_query_params now require a valid "PostgreSQL link" which causes a TypeError to be thrown. Previously the readonly slave was failing to connect for part of this test, and therefore not getting any read queries, hence the test passed.
Since PHP 8.0, all type hints are enforced in the postgres driver, which means that pg_query and friends (and most other functions too) now require a valid Postgres resource. You cannot simple use a string, and nor can you use a File stream resource.
The changes you have made to hack the db read/write checks are fine for some situations, but not for any situation where a query can actually be called somewhere in the API.
If we modify the setup to use an additional connection to the DB as a slave, then this test fails because it is performing a SELECT on the slave when creating the temp tables.
That's happening in fairly deep down when determining the index names for the table in https://github.com/moodle/moodle/blob/master/lib/ddl/postgres_sql_generator.php#L458-L492
This function uses get_record_sql on $this->mdb, which in-turn calls hits select_db_handle() and then can_use_readonly(). In can_use_readonly it's being passed a type of SQL_QUERY_SELECT because the standard get_record* functions are used. It's trying to fetch the list of table names using $this->table_names(), but it doesn't get any because they don't have the prefix, and as a result can_use_readonly() returns true, and we can use the read-only clone. https://github.com/moodle/moodle/blob/master/lib/dml/moodle_read_slave_trait.php#L309-L318
I've created a patch to make the unit test use a read DB handle, which immediately demonstrates the issue. See [^0001-70965-wip.patch]. I'm not sure whether this patch is core-suitable as it will require a second DB connection to perform tests.
This is a potentially serious bug with the read-only DB slave implementation.
Pinging brendanheywood, srdjan for their thoughts. Also stronk7.
My initial thought is that any DDL operation must not use any standard $DB operations, or alternatively that the can_use_readonly() function needs to be able to detect the use of meta-tables in a standard query, and treat it as SQL_QUERY_AUX to direct queries only to the master.
- Discovered while testing
-
MDL-70965 test_moodle_read_slave_trait sets db handle to string
-
- Closed
-