diff --git a/CHANGELOG.md b/CHANGELOG.md index 5594e1e..ed5e7df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ * perf: Stream the slow query log a line at a time during import (`fgets()`) instead of slurping the whole file into memory with `file()`, so importing a multi-gigabyte slow query log no longer costs many times its own size in PHP array overhead - the detail rows were already flushed to the database in `SLOWLOG_IMPORT_BATCH_SIZE` batches as parsing proceeds * bug: Distinguish a read error from end-of-file when streaming the import - `fgets()` returns `false` for both, so a mid-file I/O error was being treated as a clean EOF and the partial import was finalized (and post-processed) as successful. `import_logfile()` now checks `feof()` before closing the handle and fails the import (status 3, "Read Error - Import Aborted, Log May Be Truncated") instead of ingesting a truncated log +* perf: Drop the `SELECT DISTINCT sld.*` from the details list query - the association tables' composite primary keys already make every `plugin_slowlog_details` x method x table join row unique, so the DISTINCT never removed a row and only forced a filesort/hash over the `mediumtext` query columns on every page of the Details view +* refactor: Remove the write-only `plugin_slowlog_tables` table - it was inserted into during import and deleted on remove/reprocess but never read (the details list, charts, and filters all use `plugin_slowlog_details_tables`); it is no longer created, and the uninstall drop is kept so existing installs clean it up +* refactor: De-normalize the per-query method classification - `plugin_slowlog_details_methods` now stores the method name directly in a `method` varchar column instead of a `methodid` foreign key into `plugin_slowlog_methods`, and the classifier fragment dictionary moved into a PHP constant (`SLOWLOG_METHOD_FRAGMENTS`). The stats, By Method chart, details list, method filter dropdown, and chart-scope queries no longer join `plugin_slowlog_methods`, removing a join from every one of those read paths. The now-unused `plugin_slowlog_methods` dictionary table is left in place (a clean install drops it) +* bug: Migrate an existing `plugin_slowlog_details_methods` table in place during upgrade - `api_plugin_db_table_create()` never retrofits a table that already exists, so on an existing install the de-normalization above would otherwise leave the old `methodid` column (and no `method` column), breaking every import/detail/stat/chart query with an unknown-column error. `slowlog_check_upgrade()` now runs an idempotent `slowlog_migrate_details_methods_to_method()` that adds the `method` column, backfills the names from the `plugin_slowlog_methods` dictionary, then swaps the primary key to `(logid, logentry, method)` and drops `methodid`. It runs on every page (self-gating on the `methodid` column) because this schema change ships without a version bump, so the version-drift upgrade branch would never fire for it * refactor: Move the slowlog_functions.php library file into includes/ and switch every file inclusion from include/include_once to require/require_once for fail-fast consistency (references updated across setup.php, slowlog.php, import_log.php, and the test suite) * refactor: Move all schema management (table definitions, create, upgrade, and drop helpers) into includes/database.php (the thold model) with the Cacti copyright header; setup.php's install/uninstall/upgrade paths now require that file and delegate to it, keeping the api_plugin_db_table_create() create path and the db_update_table() upgrade refresh unchanged * dev: Enforce patch coverage of changed lines in CI and remove the inert COMPOSER_ROOT_VERSION env from the Pest step diff --git a/includes/database.php b/includes/database.php index 63bf23a..2a33eb0 100644 --- a/includes/database.php +++ b/includes/database.php @@ -145,8 +145,8 @@ function slowlog_setup_table_new(): void { $data['columns'][] = ['name' => 'id', 'type' => 'int(10)', 'unsigned' => true, 'NULL' => false, 'auto_increment' => true]; $data['columns'][] = ['name' => 'logid', 'type' => 'int(10)', 'unsigned' => true, 'NULL' => false]; $data['columns'][] = ['name' => 'logentry', 'type' => 'int(10)', 'unsigned' => true, 'NULL' => false]; - $data['columns'][] = ['name' => 'methodid', 'type' => 'int(10)', 'unsigned' => true, 'NULL' => false]; - $data['primary'] = ['logid', 'logentry', 'methodid']; + $data['columns'][] = ['name' => 'method', 'type' => 'varchar(45)', 'NULL' => false]; + $data['primary'] = ['logid', 'logentry', 'method']; $data['keys'][] = ['name' => 'id', 'columns' => ['id']]; $engine = slowlog_get_storage_engine(); $data['type'] = $engine; @@ -183,20 +183,11 @@ function slowlog_setup_table_new(): void { // inserting the longer ANALYZES/OPTIMIZES/CREATES seed fragments. db_execute('ALTER TABLE plugin_slowlog_methods MODIFY COLUMN `query` varchar(96) NOT NULL'); - $data = []; - $data['columns'][] = ['name' => 'logid', 'type' => 'int(10)', 'unsigned' => true, 'NULL' => false]; - $data['columns'][] = ['name' => 'table_name', 'type' => 'varchar(45)', 'NULL' => false]; - $data['primary'] = ['logid', 'table_name']; - $data['type'] = 'InnoDB'; - $data['row_format'] = 'Dynamic'; - - api_plugin_db_table_create('slowlog', 'plugin_slowlog_tables', $data); - // Schema groundwork: dictionary of every distinct table name seen across imports, plus - // whether it's a known Cacti table. Per-logentry table associations are still written - // directly to plugin_slowlog_tables/plugin_slowlog_details_tables by import_post_process(); - // this table only caches the is_cacti_table lookup so OTHER TABLES classification doesn't - // re-derive it per logentry (see slowlog_sync_table_dictionary()/slowlog_classify_other_tables()). + // whether it's a known Cacti table. Per-logentry table associations are written directly + // to plugin_slowlog_details_tables by import_post_process(); this table only caches the + // is_cacti_table lookup so OTHER TABLES classification doesn't re-derive it per logentry + // (see slowlog_sync_table_dictionary()/slowlog_classify_other_tables()). $data = []; $data['columns'][] = ['name' => 'tableid', 'type' => 'int(10)', 'unsigned' => true, 'NULL' => false, 'auto_increment' => true]; $data['columns'][] = ['name' => 'table_name', 'type' => 'varchar(45)', 'NULL' => false, 'default' => '']; @@ -315,6 +306,44 @@ function slowlog_upgrade_tables(): void { } } +/** + * Idempotently migrates an existing plugin_slowlog_details_methods table from the + * pre-2.6 (logid, logentry, methodid) layout to the de-normalized (logid, logentry, + * method varchar) layout: adds the method column, backfills the names from the + * plugin_slowlog_methods dictionary, then swaps the primary key and drops methodid. + * A no-op on fresh installs (which already receive the new schema from the create + * path) and on installs that have already been migrated, so it is safe to call on + * every page. Runs independently of the plugin version check in + * slowlog_check_upgrade() because this schema change ships without a version bump, + * and api_plugin_db_table_create() never retrofits an existing table. Called from + * slowlog_check_upgrade(). + * + * @return void + */ +function slowlog_migrate_details_methods_to_method(): void { + // Only pre-2.6 installs still carry methodid; once it's gone the migration is complete. + if (!db_table_exists('plugin_slowlog_details_methods') || !db_column_exists('plugin_slowlog_details_methods', 'methodid')) { + return; + } + + if (!db_column_exists('plugin_slowlog_details_methods', 'method')) { + db_execute('ALTER TABLE plugin_slowlog_details_methods + ADD COLUMN `method` varchar(45) NOT NULL DEFAULT \'\' AFTER `logentry`'); + } + + // Backfill the human-readable name from the dictionary for any row still missing it. + db_execute('UPDATE plugin_slowlog_details_methods AS sldm + INNER JOIN plugin_slowlog_methods AS sm ON sm.methodid = sldm.methodid + SET sldm.method = sm.method + WHERE sldm.method = \'\''); + + // Swap (logid, logentry, methodid) -> (logid, logentry, method) and retire methodid. + db_execute('ALTER TABLE plugin_slowlog_details_methods + DROP PRIMARY KEY, + ADD PRIMARY KEY (`logid`, `logentry`, `method`), + DROP COLUMN `methodid`'); +} + /** * Drops every table this plugin owns. Called from * plugin_slowlog_uninstall() when the plugin is removed. diff --git a/includes/slowlog_functions.php b/includes/slowlog_functions.php index f9ccb74..3a5bd5e 100644 --- a/includes/slowlog_functions.php +++ b/includes/slowlog_functions.php @@ -38,7 +38,6 @@ function api_slowlog_remove(int $logid): void { db_execute_prepared('DELETE FROM plugin_slowlog WHERE logid = ?', [$logid]); db_execute_prepared('DELETE FROM plugin_slowlog_details WHERE logid = ?', [$logid]); - db_execute_prepared('DELETE FROM plugin_slowlog_tables WHERE logid = ?', [$logid]); db_execute_prepared('DELETE FROM plugin_slowlog_details_tables WHERE logid = ?', [$logid]); db_execute_prepared('DELETE FROM plugin_slowlog_details_methods WHERE logid = ?', [$logid]); db_execute_prepared('DELETE FROM plugin_slowlog_stats WHERE logid = ?', [$logid]); @@ -107,8 +106,8 @@ function slowlog_bulk_insert_method_rows(array $rows): void { return; } - $sql_prefix = 'INSERT INTO plugin_slowlog_details_methods (logid, logentry, methodid) VALUES '; - $sql_suffix = ' ON DUPLICATE KEY UPDATE methodid=VALUES(methodid)'; + $sql_prefix = 'INSERT INTO plugin_slowlog_details_methods (logid, logentry, method) VALUES '; + $sql_suffix = ' ON DUPLICATE KEY UPDATE method=VALUES(method)'; foreach (array_chunk($rows, 500) as $chunk) { $placeholders = []; @@ -118,7 +117,7 @@ function slowlog_bulk_insert_method_rows(array $rows): void { $placeholders[] = '(?, ?, ?)'; $params[] = (int) $row[0]; $params[] = (int) $row[1]; - $params[] = (int) $row[2]; + $params[] = (string) $row[2]; } db_execute_prepared($sql_prefix . implode(', ', $placeholders) . $sql_suffix, $params); @@ -168,6 +167,45 @@ function slowlog_bulk_insert_table_rows(array $rows): void { // number of plugin_slowlog_details rows accumulated per bulk INSERT during import const SLOWLOG_IMPORT_BATCH_SIZE = 1000; +// The SQL-method classifier dictionary: method name => query-text fragments matched +// case-insensitively (stripos) when classifying each imported query. Replaces the former +// plugin_slowlog_methods table lookup now that the method name is stored directly in +// plugin_slowlog_details_methods.method. OTHERS (matched-nothing fallback) and OTHER TABLES +// (classified by table recognition, not a text fragment) are handled separately. +const SLOWLOG_METHOD_FRAGMENTS = [ + 'INSERTS' => ['INSERT INTO', 'INSERT IGNORE INTO'], + 'REPLACES' => ['REPLACE INTO', 'REPLACE IGNORE INTO'], + 'DELETES' => ['DELETE '], + 'SELECTS' => ['SELECT '], + 'DISTINCTS' => ['SELECT DISTINCT'], + 'UNIONS' => ['UNION'], + 'JOINS' => ['JOIN '], + 'UPDATES' => ['UPDATE '], + 'RENAMES' => ['RENAME TABLE'], + 'FLUSHES' => ['FLUSH TABLE'], + 'TRUNCATES' => ['TRUNCATE '], + 'LOAD DATA' => ['LOAD DATA INFILE '], + 'OUTFILES' => ['INTO OUTFILE '], + 'INFILES' => ['INFILE '], + 'GROUP BY' => ['GROUP BY '], + 'COUNTS' => ['COUNT('], + 'SHOWS' => ['SHOW '], + 'UNION ALLS' => ['UNION ALL'], + 'MAX_EXECUTION_TIME' => ['MAX_EXECUTION_TIME('], + 'MAX_STATEMENT_TIME' => ['MAX_STATEMENT_TIME'], + 'FORCE INDEX' => ['FORCE INDEX'], + 'ALTERS' => ['ALTER TABLE'], + 'DROPS' => ['DROP TABLE', 'DROP TEMPORARY TABLE'], + 'ANALYZES' => ['ANALYZE TABLE', 'ANALYZE NO_WRITE_TO_BINLOG TABLE', 'ANALYZE LOCAL TABLE'], + 'OPTIMIZES' => ['OPTIMIZE TABLE', 'OPTIMIZE NO_WRITE_TO_BINLOG TABLE', 'OPTIMIZE LOCAL TABLE'], + 'CREATES' => ['create table'], + 'CREATE TEMPS' => ['create temporary table'], +]; + +// Bucket method names stored directly in plugin_slowlog_details_methods.method. +const SLOWLOG_METHOD_OTHERS = 'OTHERS'; +const SLOWLOG_METHOD_OTHER_TABLES = 'OTHER TABLES'; + // the By Method/By Table charts' 'Top N' selectmenu options const SLOWLOG_CHART_TOP_OPTIONS = ['2', '10', '15', '20', '25', '30']; @@ -436,10 +474,9 @@ function slowlog_collect_stats_by_method(int $logid, array &$values, int $chunk_ $last_id = 0; do { - $rows = db_fetch_assoc_prepared('SELECT sldm.id, sm.method AS scope_key, + $rows = db_fetch_assoc_prepared('SELECT sldm.id, sldm.method AS scope_key, d.query_time, d.rows_sent, d.rows_examined, d.rows_affected, d.bytes_sent FROM plugin_slowlog_details_methods AS sldm - INNER JOIN plugin_slowlog_methods AS sm ON sm.methodid = sldm.methodid INNER JOIN plugin_slowlog_details AS d ON d.logid = sldm.logid AND d.logentry = sldm.logentry WHERE sldm.logid = ? AND sldm.id > ? @@ -744,15 +781,6 @@ function slowlog_sync_table_dictionary(int $logid, ?array $known_tables = null): * @return void */ function slowlog_classify_other_tables(int $logid): void { - $methodid = db_fetch_cell_prepared("SELECT methodid - FROM plugin_slowlog_methods - WHERE method = 'OTHER TABLES'", - []); - - if (!$methodid) { - return; - } - $rows = db_fetch_assoc_prepared('SELECT DISTINCT dt.logentry FROM plugin_slowlog_details_tables AS dt INNER JOIN plugin_slowlog_table_names AS tn @@ -768,7 +796,7 @@ function slowlog_classify_other_tables(int $logid): void { $method_rows = []; foreach ($rows as $row) { - $method_rows[] = [$logid, $row['logentry'], $methodid]; + $method_rows[] = [$logid, $row['logentry'], SLOWLOG_METHOD_OTHER_TABLES]; } slowlog_bulk_insert_method_rows($method_rows); @@ -791,15 +819,6 @@ function slowlog_classify_other_tables(int $logid): void { * @return void */ function slowlog_classify_other_tables_against_list(int $logid, array $reference_tables): void { - $methodid = db_fetch_cell_prepared("SELECT methodid - FROM plugin_slowlog_methods - WHERE method = 'OTHER TABLES'", - []); - - if (!$methodid) { - return; - } - $tables = db_fetch_assoc_prepared('SELECT DISTINCT table_name FROM plugin_slowlog_details_tables WHERE logid = ?', @@ -837,7 +856,7 @@ function slowlog_classify_other_tables_against_list(int $logid, array $reference $method_rows = []; foreach ($rows as $row) { - $method_rows[] = [$logid, $row['logentry'], $methodid]; + $method_rows[] = [$logid, $row['logentry'], SLOWLOG_METHOD_OTHER_TABLES]; } slowlog_bulk_insert_method_rows($method_rows); @@ -1358,23 +1377,7 @@ function import_post_process(int $logid, string $table_names = '', bool $usecact * default too), instead of one LIKE/NOT LIKE table scan per method (~20 round trips * previously for the default method dictionary). */ - $methods = db_fetch_assoc_prepared('SELECT * - FROM plugin_slowlog_methods - ORDER BY method', - []); - - $method_fragments = []; - $others_methodid = null; - - foreach ($methods as $row) { - // OTHERS is the "matched nothing else" bucket; OTHER TABLES is classified - // separately below (by table recognition, not a query text fragment). - if ($row['method'] == 'OTHERS') { - $others_methodid = $row['methodid']; - } elseif ($row['method'] != 'OTHER TABLES') { - $method_fragments[$row['methodid']] = explode(',', $row['query']); - } - } + $method_fragments = SLOWLOG_METHOD_FRAGMENTS; $method_chunk_size = 2000; $last_logentry = 0; @@ -1395,10 +1398,10 @@ function import_post_process(int $logid, string $table_names = '', bool $usecact $last_logentry = $row['logentry']; $matched = false; - foreach ($method_fragments as $methodid => $fragments) { + foreach ($method_fragments as $method => $fragments) { foreach ($fragments as $fragment) { if (stripos($row['query'], $fragment) !== false) { - $method_rows[] = [$logid, $row['logentry'], $methodid]; + $method_rows[] = [$logid, $row['logentry'], $method]; $matched = true; break; @@ -1406,8 +1409,8 @@ function import_post_process(int $logid, string $table_names = '', bool $usecact } } - if (!$matched && $others_methodid !== null) { - $method_rows[] = [$logid, $row['logentry'], $others_methodid]; + if (!$matched) { + $method_rows[] = [$logid, $row['logentry'], SLOWLOG_METHOD_OTHERS]; } } @@ -1447,11 +1450,6 @@ function import_post_process(int $logid, string $table_names = '', bool $usecact if ($total_tables > 0) { foreach ($tables as $t) { - db_execute_prepared('INSERT INTO plugin_slowlog_tables - (logid, table_name) - VALUES (?, ?)', - [$logid, $t]); - db_execute_prepared('INSERT INTO plugin_slowlog_details_tables (logid, logentry, table_name) SELECT ? AS logid, logentry, ? AS table_name FROM plugin_slowlog_details @@ -1532,7 +1530,6 @@ function import_post_process(int $logid, string $table_names = '', bool $usecact function slowlog_reprocess(int $logid, string $table_names = '', bool $usecacti = false, ?string $table_mode = null): void { db_execute_prepared('DELETE FROM plugin_slowlog_details_methods WHERE logid = ?', [$logid]); db_execute_prepared('DELETE FROM plugin_slowlog_details_tables WHERE logid = ?', [$logid]); - db_execute_prepared('DELETE FROM plugin_slowlog_tables WHERE logid = ?', [$logid]); db_execute_prepared('DELETE FROM plugin_slowlog_stats WHERE logid = ?', [$logid]); db_execute_prepared('UPDATE plugin_slowlog_details SET timeout = 0 WHERE logid = ?', [$logid]); @@ -2894,16 +2891,14 @@ function slowlog_get_chart_object_live(int $logid, string $scope, string $measur $agg = ($measure == 'count') ? 'COUNT(*)' : "SUM($measure)"; if ($scope == 'method') { - return db_fetch_assoc_prepared("SELECT sm.method AS scope_key, + return db_fetch_assoc_prepared("SELECT dm.method AS scope_key, $agg AS value FROM plugin_slowlog_details_methods AS dm INNER JOIN plugin_slowlog_details AS d ON dm.logid = d.logid AND dm.logentry = d.logentry - INNER JOIN plugin_slowlog_methods AS sm - ON dm.methodid = sm.methodid WHERE d.logid = ? - GROUP BY sm.methodid + GROUP BY dm.method ORDER BY value DESC" . $limit, [$logid]); } else { @@ -3092,11 +3087,10 @@ function slowlog_get_chart_scope_items(string $chart_type, int $id): array { $scope_items[] = slowlog_others_bucket_key($id); } else { - $scope_items = array_column(db_fetch_assoc_prepared('SELECT DISTINCT sm.method AS value + $scope_items = array_column(db_fetch_assoc_prepared('SELECT DISTINCT dm.method AS value FROM plugin_slowlog_details_methods AS dm - INNER JOIN plugin_slowlog_methods AS sm ON sm.methodid = dm.methodid WHERE dm.logid = ? - ORDER BY sm.method', + ORDER BY dm.method', [$id]), 'value'); } diff --git a/setup.php b/setup.php index 1b55ab0..d1e4f63 100644 --- a/setup.php +++ b/setup.php @@ -186,6 +186,11 @@ function slowlog_check_upgrade(): void { return; } + // Runs on every load (idempotent, self-gating on the methodid column) because the 2.6 + // methodid -> method de-normalization ships without a version bump, so the drift branch + // below would otherwise never fix an existing plugin_slowlog_details_methods table. + slowlog_migrate_details_methods_to_method(); + $current = $info['version']; $old = db_fetch_cell_prepared('SELECT version FROM plugin_config diff --git a/slowlog.php b/slowlog.php index 1b8a9f4..829debc 100644 --- a/slowlog.php +++ b/slowlog.php @@ -803,7 +803,7 @@ function slowlog_view_details(): void { $agg_by_table = false; $agg_by_method = false; $sql_join = ''; - $method = 'slm.method'; + $method = 'sldm.method'; $table = 'sldt.table_name'; if (get_request_var('table') == '-3') { // Aggregate by table @@ -816,20 +816,20 @@ function slowlog_view_details(): void { $sql_params[] = get_request_var('table'); } - // method_name (set by clicking a bar on the By Method chart, which only knows the - // method's name, not its id) takes priority over the mmethod dropdown, including its - // '-2' (aggregate/N/A) option - it always needs the slm join below, since it filters - // on slm.method directly. + // method_name (set by clicking a bar on the By Method chart) and the mmethod dropdown + // both filter on the method name now stored directly in + // plugin_slowlog_details_methods.method (sldm join below), so neither needs the old + // plugin_slowlog_methods dictionary join. $has_method_name = (get_request_var('method_name') != ''); if ($has_method_name) { - $sql_where .= ($sql_where != '' ? ' AND' : 'WHERE') . ' slm.method = ?'; + $sql_where .= ($sql_where != '' ? ' AND' : 'WHERE') . ' sldm.method = ?'; $sql_params[] = get_request_var('method_name'); } elseif (get_request_var('mmethod') == '-2') { // Aggregate by method $agg_by_method = true; } elseif (get_request_var('mmethod') != '-1') { - $sql_where .= ($sql_where != '' ? ' AND' : 'WHERE') . ' slm.methodid = ?'; + $sql_where .= ($sql_where != '' ? ' AND' : 'WHERE') . ' sldm.method = ?'; $sql_params[] = get_request_var('mmethod'); } @@ -845,14 +845,14 @@ function slowlog_view_details(): void { if (!$agg_by_method) { $sql_join .= ' LEFT JOIN plugin_slowlog_details_methods AS sldm ON sld.logid = sldm.logid - AND sld.logentry = sldm.logentry - LEFT JOIN plugin_slowlog_methods AS slm - ON sldm.methodid = slm.methodid'; + AND sld.logentry = sldm.logentry'; } else { $method = '"N/A" AS method'; } - $results = db_fetch_assoc_prepared("SELECT DISTINCT sld.*, $method, $table + // No DISTINCT: each detail/method/table join row is already unique (the association + // tables' composite PKs), so it only forced a filesort/hash over the mediumtext query columns. + $results = db_fetch_assoc_prepared("SELECT sld.*, $method, $table FROM plugin_slowlog_details AS sld $sql_join $sql_where @@ -860,7 +860,7 @@ function slowlog_view_details(): void { $sql_limit", $sql_params); - // cacti_log(vsprintf(str_replace('?', "'%s'", "SELECT DISTINCT sld.*, $method, $table + // cacti_log(vsprintf(str_replace('?', "'%s'", "SELECT sld.*, $method, $table // FROM plugin_slowlog_details AS sld // $sql_join // $sql_where @@ -2101,15 +2101,11 @@ function slowlog_details_filter(): void { ' . html_escape($m['method']) . ''; - } + $methods = array_merge(array_keys(SLOWLOG_METHOD_FRAGMENTS), [SLOWLOG_METHOD_OTHERS, SLOWLOG_METHOD_OTHER_TABLES]); + sort($methods); + + foreach ($methods as $m) { + print ''; } ?> diff --git a/tests/Integration/ImportPostProcessMethodsTest.php b/tests/Integration/ImportPostProcessMethodsTest.php index 7b7d543..6d5431f 100644 --- a/tests/Integration/ImportPostProcessMethodsTest.php +++ b/tests/Integration/ImportPostProcessMethodsTest.php @@ -45,7 +45,7 @@ function slowlog_test_method_insert_tuples(): array { foreach ($GLOBALS['__test_db_calls'] as $call) { if ($call['fn'] === 'db_execute_prepared' && strpos($call['sql'], 'plugin_slowlog_details_methods') !== false) { foreach (array_chunk($call['params'], 3) as $t) { - $tuples[] = array((int) $t[0], (int) $t[1], (int) $t[2]); + $tuples[] = array((int) $t[0], (int) $t[1], (string) $t[2]); } } } @@ -81,37 +81,6 @@ function slowlog_test_stats_insert_tuples(): array { TestCase::loadPluginSource('includes/slowlog_functions.php'); slowlog_test_mock_db('db_fetch_cell_prepared', 'COUNT(*)', 3); - - slowlog_test_mock_db('db_fetch_assoc_prepared', 'FROM plugin_slowlog_methods', array( - array('method' => 'INSERTS', 'query' => 'INSERT INTO,INSERT IGNORE INTO', 'methodid' => 1), - array('method' => 'REPLACES', 'query' => 'REPLACE INTO,REPLACE IGNORE INTO', 'methodid' => 2), - array('method' => 'DELETES', 'query' => 'DELETE ', 'methodid' => 3), - array('method' => 'SELECTS', 'query' => 'SELECT ', 'methodid' => 4), - array('method' => 'DISTINCTS', 'query' => 'SELECT DISTINCT', 'methodid' => 5), - array('method' => 'UNIONS', 'query' => 'UNION', 'methodid' => 6), - array('method' => 'JOINS', 'query' => 'JOIN ', 'methodid' => 7), - array('method' => 'OTHERS', 'query' => 'OTHERS', 'methodid' => 8), - array('method' => 'UPDATES', 'query' => 'UPDATE ', 'methodid' => 9), - array('method' => 'RENAMES', 'query' => 'RENAME TABLE', 'methodid' => 10), - array('method' => 'FLUSHES', 'query' => 'FLUSH TABLE', 'methodid' => 11), - array('method' => 'TRUNCATES', 'query' => 'TRUNCATE ', 'methodid' => 12), - array('method' => 'LOAD DATA', 'query' => 'LOAD DATA INFILE ', 'methodid' => 13), - array('method' => 'OUTFILES', 'query' => 'INTO OUTFILE ', 'methodid' => 14), - array('method' => 'INFILES', 'query' => 'INFILE ', 'methodid' => 15), - array('method' => 'GROUP BY', 'query' => 'GROUP BY ', 'methodid' => 16), - array('method' => 'COUNTS', 'query' => 'COUNT(', 'methodid' => 17), - array('method' => 'SHOWS', 'query' => 'SHOW ', 'methodid' => 18), - array('method' => 'UNION ALLS', 'query' => 'UNION ALL', 'methodid' => 19), - array('method' => 'MAX_EXECUTION_TIME', 'query' => 'MAX_EXECUTION_TIME(', 'methodid' => 20), - array('method' => 'MAX_STATEMENT_TIME', 'query' => 'MAX_STATEMENT_TIME', 'methodid' => 21), - array('method' => 'FORCE INDEX', 'query' => 'FORCE INDEX', 'methodid' => 23), - array('method' => 'ALTERS', 'query' => 'ALTER TABLE', 'methodid' => 24), - array('method' => 'DROPS', 'query' => 'DROP TABLE,DROP TEMPORARY TABLE', 'methodid' => 25), - array('method' => 'ANALYZES', 'query' => 'ANALYZE TABLE,ANALYZE NO_WRITE_TO_BINLOG TABLE,ANALYZE LOCAL TABLE', 'methodid' => 26), - array('method' => 'OPTIMIZES', 'query' => 'OPTIMIZE TABLE,OPTIMIZE NO_WRITE_TO_BINLOG TABLE,OPTIMIZE LOCAL TABLE', 'methodid' => 27), - array('method' => 'CREATES', 'query' => 'create table', 'methodid' => 28), - array('method' => 'CREATE TEMPS', 'query' => 'create temporary table', 'methodid' => 29), - )); }); it('classifies a row that matches a simple method', function () { @@ -121,7 +90,7 @@ function slowlog_test_stats_insert_tuples(): array { import_post_process(1, 'accounts'); - expect(slowlog_test_method_insert_tuples())->toBe(array(array(1, 1, 4))); + expect(slowlog_test_method_insert_tuples())->toBe(array(array(1, 1, 'SELECTS'))); }); it('classifies a row matching either alternative of a comma-separated method', function () { @@ -132,17 +101,17 @@ function slowlog_test_stats_insert_tuples(): array { import_post_process(1, 'accounts'); - expect(slowlog_test_method_insert_tuples())->toBe(array(array(1, 1, 1), array(1, 2, 1))); + expect(slowlog_test_method_insert_tuples())->toBe(array(array(1, 1, 'INSERTS'), array(1, 2, 'INSERTS'))); }); -it('gives a row multiple methodid rows when it matches more than one method', function () { +it('gives a row multiple method rows when it matches more than one method', function () { slowlog_test_mock_db('db_fetch_assoc_prepared', 'SELECT logentry, query', array( array('logentry' => 1, 'query' => 'select a.id from a join b on a.id=b.id'), )); import_post_process(1, 'accounts'); - expect(slowlog_test_method_insert_tuples())->toBe(array(array(1, 1, 4), array(1, 1, 7))); + expect(slowlog_test_method_insert_tuples())->toBe(array(array(1, 1, 'SELECTS'), array(1, 1, 'JOINS'))); }); it('buckets a row matching no other method as OTHERS', function () { @@ -152,7 +121,7 @@ function slowlog_test_stats_insert_tuples(): array { import_post_process(1, 'accounts'); - expect(slowlog_test_method_insert_tuples())->toBe(array(array(1, 1, 8))); + expect(slowlog_test_method_insert_tuples())->toBe(array(array(1, 1, 'OTHERS'))); }); it('classifies rows matching each of the method rows added by this PR', function () { @@ -172,13 +141,13 @@ function slowlog_test_stats_insert_tuples(): array { // Every query here also matches other, pre-existing methods (e.g. SELECTS) - the // point is just that the new method dictionary rows are actually reachable. - expect($tuples)->toContain(array(1, 1, 15)); // INFILES - expect($tuples)->toContain(array(1, 2, 16)); // GROUP BY - expect($tuples)->toContain(array(1, 3, 17)); // COUNTS - expect($tuples)->toContain(array(1, 4, 18)); // SHOWS - expect($tuples)->toContain(array(1, 5, 19)); // UNION ALLS - expect($tuples)->toContain(array(1, 6, 20)); // MAX_EXECUTION_TIME - expect($tuples)->toContain(array(1, 7, 21)); // MAX_STATEMENT_TIME + expect($tuples)->toContain(array(1, 1, 'INFILES')); // INFILES + expect($tuples)->toContain(array(1, 2, 'GROUP BY')); // GROUP BY + expect($tuples)->toContain(array(1, 3, 'COUNTS')); // COUNTS + expect($tuples)->toContain(array(1, 4, 'SHOWS')); // SHOWS + expect($tuples)->toContain(array(1, 5, 'UNION ALLS')); // UNION ALLS + expect($tuples)->toContain(array(1, 6, 'MAX_EXECUTION_TIME')); // MAX_EXECUTION_TIME + expect($tuples)->toContain(array(1, 7, 'MAX_STATEMENT_TIME')); // MAX_STATEMENT_TIME }); it('classifies a query using a FORCE INDEX hint under the FORCE INDEX method', function () { @@ -188,7 +157,7 @@ function slowlog_test_stats_insert_tuples(): array { import_post_process(1, 'accounts'); - expect(slowlog_test_method_insert_tuples())->toContain(array(1, 1, 23)); + expect(slowlog_test_method_insert_tuples())->toContain(array(1, 1, 'FORCE INDEX')); }); it('classifies rows matching each of the ALTERS/DROPS/ANALYZES/OPTIMIZES methods', function () { @@ -206,13 +175,13 @@ function slowlog_test_stats_insert_tuples(): array { $tuples = slowlog_test_method_insert_tuples(); - expect($tuples)->toContain(array(1, 1, 24)); // ALTERS - expect($tuples)->toContain(array(1, 2, 25)); // DROPS - expect($tuples)->toContain(array(1, 3, 25)); // DROPS (DROP TEMPORARY TABLE) - expect($tuples)->toContain(array(1, 4, 26)); // ANALYZES - expect($tuples)->toContain(array(1, 5, 26)); // ANALYZES (NO_WRITE_TO_BINLOG) - expect($tuples)->toContain(array(1, 6, 27)); // OPTIMIZES - expect($tuples)->toContain(array(1, 7, 27)); // OPTIMIZES (NO_WRITE_TO_BINLOG) + expect($tuples)->toContain(array(1, 1, 'ALTERS')); // ALTERS + expect($tuples)->toContain(array(1, 2, 'DROPS')); // DROPS + expect($tuples)->toContain(array(1, 3, 'DROPS')); // DROPS (DROP TEMPORARY TABLE) + expect($tuples)->toContain(array(1, 4, 'ANALYZES')); // ANALYZES + expect($tuples)->toContain(array(1, 5, 'ANALYZES')); // ANALYZES (NO_WRITE_TO_BINLOG) + expect($tuples)->toContain(array(1, 6, 'OPTIMIZES')); // OPTIMIZES + expect($tuples)->toContain(array(1, 7, 'OPTIMIZES')); // OPTIMIZES (NO_WRITE_TO_BINLOG) }); it('classifies a permanent CREATE TABLE under CREATES but not CREATE TEMPS', function () { @@ -224,8 +193,8 @@ function slowlog_test_stats_insert_tuples(): array { $tuples = slowlog_test_method_insert_tuples(); - expect($tuples)->toContain(array(1, 1, 28)); // CREATES - expect($tuples)->not->toContain(array(1, 1, 29)); // CREATE TEMPS + expect($tuples)->toContain(array(1, 1, 'CREATES')); // CREATES + expect($tuples)->not->toContain(array(1, 1, 'CREATE TEMPS')); // CREATE TEMPS }); it('classifies a CREATE TEMPORARY TABLE under CREATE TEMPS but not CREATES', function () { @@ -237,8 +206,8 @@ function slowlog_test_stats_insert_tuples(): array { $tuples = slowlog_test_method_insert_tuples(); - expect($tuples)->toContain(array(1, 1, 29)); // CREATE TEMPS - expect($tuples)->not->toContain(array(1, 1, 28)); // CREATES + expect($tuples)->toContain(array(1, 1, 'CREATE TEMPS')); // CREATE TEMPS + expect($tuples)->not->toContain(array(1, 1, 'CREATES')); // CREATES }); it('inserts the method classification with a single batched statement', function () { @@ -255,18 +224,12 @@ function slowlog_test_stats_insert_tuples(): array { })); expect($calls)->toHaveCount(1); - expect($calls[0]['sql'])->toContain('ON DUPLICATE KEY UPDATE methodid=VALUES(methodid)'); + expect($calls[0]['sql'])->toContain('ON DUPLICATE KEY UPDATE method=VALUES(method)'); }); -it('records the table dictionary row and its detail associations', function () { +it('records the detail table associations', function () { import_post_process(1, 'accounts users'); - $dictionary = slowlog_test_prepared_calls_matching($GLOBALS['__test_db_calls'], 'INSERT INTO plugin_slowlog_tables'); - expect(array_column($dictionary, 'params'))->toBe(array( - array(1, 'accounts'), - array(1, 'users'), - )); - $details = slowlog_test_prepared_calls_matching($GLOBALS['__test_db_calls'], 'INSERT INTO plugin_slowlog_details_tables'); expect($details)->toHaveCount(2); expect($details[0]['params'])->toBe(array( diff --git a/tests/Integration/ReprocessTest.php b/tests/Integration/ReprocessTest.php index 1a3249e..21cc5b2 100644 --- a/tests/Integration/ReprocessTest.php +++ b/tests/Integration/ReprocessTest.php @@ -51,7 +51,6 @@ function slowlog_test_calls_matching(array $calls, string $needle): array { expect(slowlog_test_calls_matching($calls, 'DELETE FROM plugin_slowlog_details_methods'))->toHaveCount(1); expect(slowlog_test_calls_matching($calls, 'DELETE FROM plugin_slowlog_details_tables'))->toHaveCount(1); - expect(slowlog_test_calls_matching($calls, 'DELETE FROM plugin_slowlog_tables'))->toHaveCount(1); expect(slowlog_test_calls_matching($calls, 'DELETE FROM plugin_slowlog_stats'))->toHaveCount(1); expect(slowlog_test_calls_matching($calls, 'SET timeout = 0'))->toHaveCount(1); diff --git a/tests/Security/PreparedStatementUsageTest.php b/tests/Security/PreparedStatementUsageTest.php index e860263..0a1fbe6 100644 --- a/tests/Security/PreparedStatementUsageTest.php +++ b/tests/Security/PreparedStatementUsageTest.php @@ -62,10 +62,6 @@ expect($helpers)->not->toContain("SELECT '\$logid' AS logid, logentry"); }); - it('uses a prepared insert for plugin_slowlog_tables', function () use ($helpers) { - expect(preg_match('/db_execute_prepared\s*\(\s*\'INSERT INTO plugin_slowlog_tables/s', $helpers))->toBe(1); - }); - it('uses a prepared insert for plugin_slowlog_details_tables', function () use ($helpers) { expect(preg_match('/db_execute_prepared\s*\(\s*\'INSERT INTO plugin_slowlog_details_tables/s', $helpers))->toBe(1); }); diff --git a/tests/Unit/MigrateDetailsMethodsTest.php b/tests/Unit/MigrateDetailsMethodsTest.php new file mode 100644 index 0000000..16d3f38 --- /dev/null +++ b/tests/Unit/MigrateDetailsMethodsTest.php @@ -0,0 +1,113 @@ +toBeEmpty(); +}); + +it('does nothing when the methodid column is already gone', function () { + slowlog_test_mock_db('db_table_exists', 'plugin_slowlog_details_methods', true); + // db_column_exists defaults to false, so methodid is reported absent (already migrated). + + slowlog_migrate_details_methods_to_method(); + + expect(slowlog_test_migration_executes())->toBeEmpty(); +}); + +it('adds the method column, backfills from the dictionary, and swaps the primary key', function () { + slowlog_test_mock_db('db_table_exists', 'plugin_slowlog_details_methods', true); + slowlog_test_mock_db('db_column_exists', function ($sql, $params) { + return $params[1] === 'methodid'; + }, true); + + slowlog_migrate_details_methods_to_method(); + + $executes = slowlog_test_migration_executes(); + + $addColumn = array_filter($executes, function ($call) { + return stripos($call['sql'], 'ADD COLUMN `method`') !== false; + }); + expect($addColumn)->toHaveCount(1); + + $backfill = array_filter($executes, function ($call) { + return stripos($call['sql'], 'UPDATE plugin_slowlog_details_methods') !== false + && stripos($call['sql'], 'JOIN plugin_slowlog_methods') !== false; + }); + expect($backfill)->toHaveCount(1); + + $swapKey = array_filter($executes, function ($call) { + return stripos($call['sql'], 'DROP PRIMARY KEY') !== false + && stripos($call['sql'], 'DROP COLUMN `methodid`') !== false; + }); + expect($swapKey)->toHaveCount(1); +}); + +it('skips adding the method column when it already exists', function () { + slowlog_test_mock_db('db_table_exists', 'plugin_slowlog_details_methods', true); + slowlog_test_mock_db('db_column_exists', function ($sql, $params) { + return in_array($params[1], array('methodid', 'method'), true); + }, true); + + slowlog_migrate_details_methods_to_method(); + + $executes = slowlog_test_migration_executes(); + + $addColumn = array_filter($executes, function ($call) { + return stripos($call['sql'], 'ADD COLUMN `method`') !== false; + }); + expect($addColumn)->toBeEmpty(); + + $backfill = array_filter($executes, function ($call) { + return stripos($call['sql'], 'UPDATE plugin_slowlog_details_methods') !== false; + }); + expect($backfill)->toHaveCount(1); + + $swapKey = array_filter($executes, function ($call) { + return stripos($call['sql'], 'DROP PRIMARY KEY') !== false; + }); + expect($swapKey)->toHaveCount(1); +}); diff --git a/tests/Unit/OtherTablesClassificationTest.php b/tests/Unit/OtherTablesClassificationTest.php index 621c9dd..7e4a521 100644 --- a/tests/Unit/OtherTablesClassificationTest.php +++ b/tests/Unit/OtherTablesClassificationTest.php @@ -40,7 +40,7 @@ function slowlog_test_method_tuples(): array { foreach (slowlog_test_method_calls() as $call) { foreach (array_chunk($call['params'], 3) as $t) { - $tuples[] = array((int) $t[0], (int) $t[1], (int) $t[2]); + $tuples[] = array((int) $t[0], (int) $t[1], (string) $t[2]); } } @@ -50,8 +50,6 @@ function slowlog_test_method_tuples(): array { beforeEach(function () { TestCase::loadPluginSource('includes/slowlog_functions.php'); - - slowlog_test_mock_db('db_fetch_cell_prepared', "WHERE method = 'OTHER TABLES'", 22); }); it('tags a logentry that touches a non-Cacti table', function () { @@ -61,18 +59,7 @@ function slowlog_test_method_tuples(): array { slowlog_classify_other_tables(1); - expect(slowlog_test_method_tuples())->toBe(array(array(1, 2, 22))); -}); - -it('does nothing when the OTHER TABLES method row does not exist', function () { - slowlog_test_mock_db('db_fetch_cell_prepared', "WHERE method = 'OTHER TABLES'", 0); - slowlog_test_mock_db('db_fetch_assoc_prepared', 'SELECT DISTINCT dt.logentry', array( - array('logentry' => 2), - )); - - slowlog_classify_other_tables(1); - - expect(slowlog_test_method_calls())->toBe(array()); + expect(slowlog_test_method_tuples())->toBe(array(array(1, 2, 'OTHER TABLES'))); }); it('does nothing when every table is a recognized Cacti table', function () { @@ -85,11 +72,6 @@ function slowlog_test_method_tuples(): array { it('is only invoked from import_post_process() when usecacti is used', function () { slowlog_test_mock_db('db_fetch_cell_prepared', 'COUNT(*)', 3); - slowlog_test_mock_db('db_fetch_assoc_prepared', 'FROM plugin_slowlog_methods', array( - array('method' => 'SELECTS', 'query' => 'SELECT ', 'methodid' => 4), - array('method' => 'OTHERS', 'query' => 'OTHERS', 'methodid' => 8), - array('method' => 'OTHER TABLES', 'query' => 'OTHER TABLES', 'methodid' => 22), - )); slowlog_test_mock_db('db_fetch_assoc_prepared', 'SELECT logentry, query', array( array('logentry' => 1, 'query' => 'select * from mystery_tbl'), )); @@ -99,16 +81,14 @@ function slowlog_test_method_tuples(): array { import_post_process(1, 'mystery_tbl', false); - expect(slowlog_test_method_tuples())->toBe(array(array(1, 1, 4))); + expect(slowlog_test_method_tuples())->toBe(array(array(1, 1, 'SELECTS'))); }); -it("does not let the 'OTHER TABLES' fragment leak into ordinary query-text matching", function () { - // If 'OTHER TABLES' were treated like a normal fragment-matched method, stripos() against - // its own (never-appearing-in-real-SQL) fragment would simply never match - this proves it - // is excluded from that loop entirely rather than relying on it coincidentally not matching. - $source = file_get_contents(realpath(__DIR__ . '/../../includes/slowlog_functions.php')); - - expect($source)->toContain("\$row['method'] != 'OTHER TABLES'"); +it("does not let the 'OTHER TABLES' / 'OTHERS' buckets leak into ordinary query-text matching", function () { + // Both are classified separately (OTHER TABLES by table recognition, OTHERS as the + // matched-nothing fallback), so neither may appear as a fragment-matched method. + expect(array_key_exists('OTHER TABLES', SLOWLOG_METHOD_FRAGMENTS))->toBeFalse(); + expect(array_key_exists('OTHERS', SLOWLOG_METHOD_FRAGMENTS))->toBeFalse(); }); /* @@ -128,7 +108,7 @@ function slowlog_test_method_tuples(): array { slowlog_classify_other_tables_against_list(1, array('host')); - expect(slowlog_test_method_tuples())->toBe(array(array(1, 2, 22))); + expect(slowlog_test_method_tuples())->toBe(array(array(1, 2, 'OTHER TABLES'))); }); it('does nothing when every table is in the supplied reference list', function () { diff --git a/tests/Unit/SetupTableApiUsageTest.php b/tests/Unit/SetupTableApiUsageTest.php index 195b459..d5b5dde 100644 --- a/tests/Unit/SetupTableApiUsageTest.php +++ b/tests/Unit/SetupTableApiUsageTest.php @@ -62,7 +62,6 @@ function slowlog_test_calls_to(array $calls, string $fn): array { 'plugin_slowlog_details_methods', 'plugin_slowlog_details_tables', 'plugin_slowlog_methods', - 'plugin_slowlog_tables', 'plugin_slowlog_table_names', 'plugin_slowlog_reserved_words', 'plugin_slowlog_stats', @@ -170,7 +169,7 @@ function slowlog_test_calls_to(array $calls, string $fn): array { $tables = slowlog_test_calls_to($GLOBALS['__test_db_calls'], 'api_plugin_db_table_create'); - expect($tables)->toHaveCount(18); + expect($tables)->toHaveCount(16); }); it('drops every plugin table on uninstall, including the stats cache', function () { diff --git a/tests/Unit/SlowlogRemoveTest.php b/tests/Unit/SlowlogRemoveTest.php index 0fb21b8..db7b207 100644 --- a/tests/Unit/SlowlogRemoveTest.php +++ b/tests/Unit/SlowlogRemoveTest.php @@ -41,7 +41,6 @@ expect($tables)->toBe(array( 'plugin_slowlog', 'plugin_slowlog_details', - 'plugin_slowlog_tables', 'plugin_slowlog_details_tables', 'plugin_slowlog_details_methods', 'plugin_slowlog_stats', diff --git a/tests/Unit/TableModeDispatchTest.php b/tests/Unit/TableModeDispatchTest.php index 3d67573..80cac5a 100644 --- a/tests/Unit/TableModeDispatchTest.php +++ b/tests/Unit/TableModeDispatchTest.php @@ -31,8 +31,12 @@ if (!function_exists('slowlog_test_table_inserts')) { function slowlog_test_table_inserts(): array { + // List mode writes table associations via the LIKE-scan INSERT ... SELECT; the + // tokenizer modes use the VALUES form, so match on the LIKE scan to detect list mode. return array_values(array_filter($GLOBALS['__test_db_calls'], function ($call) { - return $call['fn'] === 'db_execute_prepared' && strpos($call['sql'], 'INSERT INTO plugin_slowlog_tables') !== false; + return $call['fn'] === 'db_execute_prepared' + && strpos($call['sql'], 'INSERT INTO plugin_slowlog_details_tables') !== false + && strpos($call['sql'], 'query LIKE') !== false; })); } @@ -88,10 +92,7 @@ function slowlog_test_other_tables_call_count(): int { it('uses the explicit table list when table_names is given (legacy behavior)', function () { import_post_process(1, 'accounts users', false); - expect(array_column(slowlog_test_table_inserts(), 'params'))->toBe(array( - array(1, 'accounts'), - array(1, 'users'), - )); + expect(array_map(function ($call) { return $call['params'][1]; }, slowlog_test_table_inserts()))->toBe(array('accounts', 'users')); expect(slowlog_test_other_tables_call_count())->toBe(0); }); diff --git a/tests/bootstrap-unit.php b/tests/bootstrap-unit.php index abe64f2..b49e838 100644 --- a/tests/bootstrap-unit.php +++ b/tests/bootstrap-unit.php @@ -207,7 +207,7 @@ function db_index_exists($table, $index) { if (!function_exists('db_column_exists')) { function db_column_exists($table, $column) { - return false; + return slowlog_test_db_result('db_column_exists', $table, array($table, $column), false); } }