Michael Widenius added a comment - 2024-02-20 15:12 From the commit message: Updated query_response_time plugin This is to update the plugin to be compatible with Percona's query_response_time plugin, with some additions. Some of the tests are taken from Percona server. Added plugins QUERY_RESPONSE_TIME_READ, QUERY_RESPONSE_TIME_WRITE and QUERY_RESPONSE_TIME_READ_WRITE. Added option session_stats. Notes: All modules are dependent on QUERY_RESPONSE_READ_TIME. This must always be enabled if any of the other modules are used. This will be auto-enabled in the near future. Accounting are done per statement. Stored functions are regarded as part of the original statement. For stored procedures the accounting are done per statement executed in the stored procedure CALL will not be accounted for because of this. FLUSH commands will not be accounted for. This is to ensure that FLUSH QUERY_RESPONSE_TIME is not part of the statistics. (This helps when testing with mtr and otherwise)

Sergei Golubchik added a comment - 2024-03-23 13:09 to test this feature separately please use bb-11.5- MDEV-33501 -query-response-time

Elena Stepanova added a comment - 2024-04-29 23:06 - edited Test results as of bb-11.5-MDEV-33501-query-response-time 2b35d7c If addressing the notes below doesn't cause non-trivial code changes, I have no objections against pushing the feature into 11.5 main branch and releasing with 11.5.1 RC. Must do These need to be fixed, or at least looked at, before the release: MDEV-34031 (FLUSH QUERY_RESPONSE_TIME_WRITE also flushes READ, and vice versa) MDEV-34032 (EXPLAIN <DML statement> counts as write in QUERY_RESPONSE_TIME) If it's decided that both or either are "by design", I won't strongly object it, although I found both rather counter-intuitive. Also, I was pulling some commits from bb-11.5-monty into the feature branch, it's better to check that everything there is in order. Good to have MDEV-34034 - This is a suggestion for small increase in CI coverage, it doesn't have to be done before the RC, but it's a trivial patch (included). Other assorted notes The commit comment says All modules are dependent on QUERY_RESPONSE_READ_TIME. It doesn't appear to be true. QUERY_RESPONSE_TIME_AUDIT is needed for collecting the statistics, and QUERY_RESPONSE_TIME is needed for (at least) enabling the collection, as the variables come with it; but for example QUERY_RESPONSE_TIME_WRITE works fine without QUERY_RESPONSE_TIME_READ being installed: MariaDB [test]> select plugin_name from information_schema.plugins where plugin_name like 'query_response%' ; + ---------------------------+ | plugin_name | + ---------------------------+ | QUERY_RESPONSE_TIME_AUDIT | | QUERY_RESPONSE_TIME | | QUERY_RESPONSE_TIME_WRITE | + ---------------------------+ 3 rows in set (0.006 sec)   MariaDB [test]> set global query_response_time_stats= on ; Query OK, 0 rows affected (0.000 sec)   MariaDB [test]> create table t (a int ); Query OK, 0 rows affected (0.038 sec)   MariaDB [test]> select * from information_schema.query_response_time_write where count > 0; + ----------------+-------+----------------+ | TIME | COUNT | TOTAL | + ----------------+-------+----------------+ | 0.100000 | 1 | 0.037375 | + ----------------+-------+----------------+ 1 row in set (0.001 sec) I think it's good and makes sense, hopefully the comment is wrong (in which case it's better to correct it in git and JIRA, to avoid confusion). Also, the comment says Stored functions are regarded as part of the original statement It is not necessarily so. For example, if a function contains a writing DML, it counts as a "write", even if the original statement is SELECT; and the SELECT in this case isn't counted. MariaDB [test]> create table t (a int ); Query OK, 0 rows affected (0.025 sec)   MariaDB [test]> delimiter $ MariaDB [test]> create or replace function f() returns int -> begin -> insert into t values (1); -> return 1; -> end $ Query OK, 0 rows affected (0.016 sec)   MariaDB [test]> delimiter ;   MariaDB [test]> flush query_response_time; Query OK, 0 rows affected (0.001 sec)   MariaDB [test]> select f(); + ------+ | f() | + ------+ | 1 | + ------+ 1 row in set (0.015 sec)   MariaDB [test]> select sum (read_count), sum (write_count) from information_schema.query_response_time_read_write; + -----------------+------------------+ | sum (read_count) | sum (write_count) | + -----------------+------------------+ | 0 | 1 | + -----------------+------------------+ 1 row in set (0.001 sec) Maybe this note needs to be refined somehow. For some statements, it wasn't obvious to me that they would be categorized the way they are. I don't see it a big enough issue to deserve a JIRA ticket, just some notes for documentation if it goes into such detail: COMMIT / ROLLBACK statements are "reads". It is particularly non-obvious for ROLLBACK, because the previous DML in the transaction was counted as "write", and now ROLLBACK is undoing it; PURGE BINARY LOGS and RESET MASTER are "read", while it's clearly making changes. It seems odd that a non-privileged user can enable statistics in their session, but it will affect the whole global statistics. This way, a mischievous user can flood global statistics with nonsense (and maybe even affect performance?). However, I don't have better suggestions in this regard, so it's just a side note. There was one miss in the coverage: ===File plugin/query_response_time/plugin.cc: 199 : + return 0; As I understand, it's a rare corner case (if at all achievable), so possibly it isn't necessary to make an effort to cover it. However, if it's easy enough, it would be good to do.

Elena Stepanova added a comment - 2024-05-04 17:01 A fix for MDEV-34031 was pushed into bb-11.5-monty and cherry-picked into bb-11.5- MDEV-33501 -query-response-time (two commits on top of the one described in the previous comment). Without re-running all the tests, I did a quick check of the change, it seems to be working as one could expect, and I didn't see any complaints from buildbots, so I think it can be used. MDEV-34032 was said to be a duplicate of MDEV-33752 which is to be fixed in 10.6.