Page MenuHomePhabricator

Improve query verb/flags handling in Query class for DROP/ALTER/CREATE statements
Closed, ResolvedPublic

Description

It would be useful for Query to track slightly better query verbs/flags to distinguish vastly different types of queries like DROP DATABASE, DROP TABLE and DROP INDEX. Currently, one can check getTables() to distinguish some of these (e.g. "DROP DATABASE" vs "DROP TABLE"), but not others (e.g. "DROP INDEX x ON a" vs "DROP TABLE a"). It should be easy to know if a query is a table read (DQL), table write (DML), table create, table drop, or table alter (or indexes thereof).

Also note that there can be different ways of altering things for different db engines and equivalent ways for a single engine. For example, index creation/dropping in mysql can use "ALTER TABLE a ADD index x"/"ALTER TABLE a DROP INDEX x" or "CREATE INDEX x ON TABLE a"/"DROP INDEX x ON A". It would be useful if these cases could be checked in a normalized way.

$flags is currently confusing since there are flags describing what the query does and flags that alter retry/DBO_TRX/error handling. The former could be replaced by $queryType and the later would stay in Database. QueryBuilderFromRawSql checks !$flags but there are flags other than QUERY_CHANGE_* flags that might be set. Giving $flags=0 to query() with a string $sql can cause $flags and $sql->getFlags() to diverge oddly.

I think the easiest thing is to:

  • Provide Query:: constants for the $queryVerb argument to the Query constructor. These would distinguish table/index/db DDL statements. Use these in getVerb().
  • Use the new constants in the callers (all RDBMS internal) when possible (things like "PRAGMA"/"ATTACH" would not have constants).
  • Deprecate some QUERY_ flags that aren't needed anymore

Based on comments in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/946538 and things I noticed looking at Query::getVerb() callers. Also, since 9e6fa0d2253c291e583cbe4366a6f2e41c94f958 ,getTempTableWrites() no longer distinguishes "CREATE" from "CREATE TEMPORARY" anymore (which happens to work since unit tests use TEMP_PSEUDO_PERMANENT and the only other thing sending CREATE is schema change scripts which still end up working give what sessionTempTables is used for).

I also notice that isWriteQuery() was made to return false for GET_LOCK() to avoid TransactionProfiler logging, which OK, except we lack enforcement of read-only mode on DB_REPLICA connections (which would blow up in production). executeQuery() should check QUERY_CHANGE_LOCKS or queryType().

Event Timeline

Change 976368 had a related patch set uploaded (by Aaron Schulz; author: Aaron Schulz):

[mediawiki/core@master] rdbms: improve Query/QueryBuilderFromRawSql handling of table tracking

https://gerrit.wikimedia.org/r/976368

I also notice that isWriteQuery() was made to return false for GET_LOCK() to avoid TransactionProfiler logging, which OK, except we lack enforcement of read-only mode on DB_REPLICA connections (which would blow up in production). executeQuery() should check QUERY_CHANGE_LOCKS or queryType().

Noting that lock related functions are not part of IReadableDatabase so the chance of this happening is quite slime.

Change 979437 had a related patch set uploaded (by Aaron Schulz; author: Aaron Schulz):

[mediawiki/core@master] rdbms: make Query only track tables that are the target of writes

https://gerrit.wikimedia.org/r/979437

Change 980502 had a related patch set uploaded (by Aaron Schulz; author: Aaron Schulz):

[mediawiki/core@master] rdbms: distinguish different CREATE statements in Query::getVerb()

https://gerrit.wikimedia.org/r/980502

Change 979437 merged by jenkins-bot:

[mediawiki/core@master] rdbms: make Query only track tables that are the target of writes

https://gerrit.wikimedia.org/r/979437

Change 980502 merged by jenkins-bot:

[mediawiki/core@master] rdbms: distinguish different CREATE statements in Query::getVerb()

https://gerrit.wikimedia.org/r/980502

Change 995371 had a related patch set uploaded (by Aaron Schulz; author: Aaron Schulz):

[mediawiki/core@master] rdbms: make QueryBuilderFromRawSql always set QUERY_CHANGE_* flags

https://gerrit.wikimedia.org/r/995371

Change 995371 merged by jenkins-bot:

[mediawiki/core@master] rdbms: make QueryBuilderFromRawSql always set QUERY_CHANGE_* flags

https://gerrit.wikimedia.org/r/995371

FJoseph-WMF changed the task status from Open to In Progress.Apr 5 2024, 2:44 PM

Change #1021520 had a related patch set uploaded (by Aaron Schulz; author: Aaron Schulz):

[mediawiki/core@master] rdbms: remove unused QUERY_CREATE_TEMP constant

https://gerrit.wikimedia.org/r/1021520

Change #1021520 merged by jenkins-bot:

[mediawiki/core@master] rdbms: remove unused QUERY_CREATE_TEMP constant

https://gerrit.wikimedia.org/r/1021520

aaron updated the task description. (Show Details)