From 73408f1b701e3dfd26e3c925693a91bb2b1985a4 Mon Sep 17 00:00:00 2001 From: Rati Rukhadze Date: Tue, 11 Aug 2026 00:01:34 +0400 Subject: [PATCH] feat: judge dropped indexes on the engine's own read counters and flag redundant ones --- config/difflock.php | 2 + src/Contracts/IndexStatistics.php | 40 ++++++ src/Database/ConnectionIndexStatistics.php | 153 +++++++++++++++++++++ src/Database/DatabaseContextFactory.php | 1 + src/Database/FixedIndexStatistics.php | 37 +++++ src/Migration/DatabaseContext.php | 20 +++ src/Migration/Rules/DropIndexRule.php | 98 ++++++++++--- src/Migration/Rules/RedundantIndexRule.php | 135 ++++++++++++++++++ tests/Unit/IndexEvidenceTest.php | 150 ++++++++++++++++++++ tests/Unit/RulesTest.php | 2 +- 10 files changed, 619 insertions(+), 19 deletions(-) create mode 100644 src/Contracts/IndexStatistics.php create mode 100644 src/Database/ConnectionIndexStatistics.php create mode 100644 src/Database/FixedIndexStatistics.php create mode 100644 src/Migration/Rules/RedundantIndexRule.php create mode 100644 tests/Unit/IndexEvidenceTest.php diff --git a/config/difflock.php b/config/difflock.php index 105cfef..fd17639 100644 --- a/config/difflock.php +++ b/config/difflock.php @@ -10,6 +10,7 @@ use Difflock\Migration\Rules\DropTableRule; use Difflock\Migration\Rules\ForeignKeyRule; use Difflock\Migration\Rules\LargeTableRule; +use Difflock\Migration\Rules\RedundantIndexRule; use Difflock\Migration\Rules\RenameColumnRule; use Difflock\Migration\Rules\SensitiveColumnRule; use Difflock\Migration\Rules\UnindexedForeignKeyRule; @@ -223,6 +224,7 @@ DropIndexRule::class, ForeignKeyRule::class, UnindexedForeignKeyRule::class, + RedundantIndexRule::class, SensitiveColumnRule::class, LargeTableRule::class, ], diff --git a/src/Contracts/IndexStatistics.php b/src/Contracts/IndexStatistics.php new file mode 100644 index 0000000..8d398e0 --- /dev/null +++ b/src/Contracts/IndexStatistics.php @@ -0,0 +1,40 @@ +|null */ + private ?array $scans = null; + + private ?int $days = null; + + public function __construct( + private readonly ConnectionResolverInterface $connections, + private readonly ?string $connection = null, + ) {} + + public function scans(string $table, string $index): ?int + { + $this->load(); + + return $this->scans[$this->key($table, $index)] ?? null; + } + + public function observedDays(): ?int + { + $this->load(); + + return $this->days; + } + + private function load(): void + { + if ($this->scans !== null) { + return; + } + + $this->scans = []; + + try { + match ($this->driver()) { + 'pgsql' => $this->loadPostgres(), + 'mysql', 'mariadb' => $this->loadMysql(), + default => null, + }; + } catch (Throwable) { + // No access to the statistics views, or the schema does not have them. + // Unknown is a legitimate answer and the rules are built for it. + $this->scans = []; + $this->days = null; + } + } + + private function loadPostgres(): void + { + $sql = 'select relname as table_name, indexrelname as index_name, idx_scan as scans ' + .'from pg_stat_user_indexes'; + + foreach ($this->connection()->select($sql) as $row) { + $this->record($row); + } + + $reset = $this->connection()->select( + 'select extract(epoch from (now() - stats_reset)) / 86400 as days ' + .'from pg_stat_database where datname = current_database()', + ); + + $days = ((array) ($reset[0] ?? []))['days'] ?? null; + + $this->days = is_numeric($days) && (int) $days >= 0 ? (int) $days : null; + } + + private function loadMysql(): void + { + $sql = 'select object_name as table_name, index_name, count_star as scans ' + .'from performance_schema.table_io_waits_summary_by_index_usage ' + .'where object_schema = database() and index_name is not null'; + + foreach ($this->connection()->select($sql) as $row) { + $this->record($row); + } + + // MySQL's performance_schema counters reset when the server restarts, and + // uptime is the closest thing to a window it will give. + $uptime = $this->connection()->select("show global status like 'Uptime'"); + $value = ((array) ($uptime[0] ?? []))['Value'] ?? null; + + $this->days = is_numeric($value) ? (int) ((int) $value / 86400) : null; + } + + private function record(mixed $row): void + { + $values = (array) $row; + + $table = $values['table_name'] ?? null; + $index = $values['index_name'] ?? null; + $scans = $values['scans'] ?? null; + + if (! is_scalar($table) || ! is_scalar($index) || ! is_numeric($scans)) { + return; + } + + $this->scans[$this->key((string) $table, (string) $index)] = (int) $scans; + } + + private function key(string $table, string $index): string + { + return strtolower($table)."\0".strtolower($index); + } + + private function driver(): string + { + try { + return $this->connection()->getDriverName(); + } catch (Throwable) { + return ''; + } + } + + private function connection(): Connection + { + $connection = $this->connections->connection($this->connection); + + return $connection instanceof Connection + ? $connection + : throw new RuntimeException('Difflock needs a database connection it can query.'); + } +} diff --git a/src/Database/DatabaseContextFactory.php b/src/Database/DatabaseContextFactory.php index 310efe2..621fad3 100644 --- a/src/Database/DatabaseContextFactory.php +++ b/src/Database/DatabaseContextFactory.php @@ -65,6 +65,7 @@ private function build(): DatabaseContext environment: $this->environment(), version: $this->version(), available: true, + indexes: new ConnectionIndexStatistics($this->connections, $this->connection), ); } diff --git a/src/Database/FixedIndexStatistics.php b/src/Database/FixedIndexStatistics.php new file mode 100644 index 0000000..bc6158f --- /dev/null +++ b/src/Database/FixedIndexStatistics.php @@ -0,0 +1,37 @@ + $scans Keyed `table.index`. + */ + public function __construct( + private array $scans = [], + private ?int $days = null, + ) {} + + public function scans(string $table, string $index): ?int + { + return $this->scans[$table.'.'.$index] ?? null; + } + + public function observedDays(): ?int + { + return $this->days; + } +} diff --git a/src/Migration/DatabaseContext.php b/src/Migration/DatabaseContext.php index 9ac58f4..27590af 100644 --- a/src/Migration/DatabaseContext.php +++ b/src/Migration/DatabaseContext.php @@ -4,7 +4,9 @@ namespace Difflock\Migration; +use Difflock\Contracts\IndexStatistics; use Difflock\Contracts\TableStatistics; +use Difflock\Database\FixedIndexStatistics; use Difflock\Schema\DatabaseSchema; use Difflock\Schema\Table; @@ -34,8 +36,26 @@ public function __construct( public string $environment = 'unknown', public ?string $version = null, public bool $available = true, + public IndexStatistics $indexes = new FixedIndexStatistics, ) {} + /** + * How many times the engine has read this index, or null when it will not say. + * + * Null is "unknown", never zero — the distinction is load-bearing for the + * drop-index rule, which would otherwise call a heavily used index unused. + */ + public function indexScans(?string $table, string $index): ?int + { + return $table === null || ! $this->available ? null : $this->indexes->scans($table, $index); + } + + /** How long the engine's index counters have been accumulating, in days. */ + public function indexObservedDays(): ?int + { + return $this->available ? $this->indexes->observedDays() : null; + } + public function driver(): ?string { return $this->schema->driver; diff --git a/src/Migration/Rules/DropIndexRule.php b/src/Migration/Rules/DropIndexRule.php index de40a32..6e5b246 100644 --- a/src/Migration/Rules/DropIndexRule.php +++ b/src/Migration/Rules/DropIndexRule.php @@ -10,22 +10,33 @@ use Difflock\Migration\MigrationFinding; use Difflock\Migration\Parser\Operation; use Difflock\Migration\Subject; +use Difflock\Migration\Thresholds; use Difflock\Risk\RiskLevel; /** * `dropIndex()`, `dropUnique()`, `dropPrimary()`. * - * The honest position here is narrow, and the rule sticks to it. Difflock has no - * query workload to consult, so it **cannot** tell you whether dropping an index - * will make anything slower — anybody claiming otherwise from a migration file alone - * is guessing. What it can tell you is the difference between the three: + * There are two separate questions here and the rule keeps them apart. * - * - `dropUnique` and `dropPrimary` remove a *constraint*. Duplicates that the - * database was rejecting a moment ago now go in, and putting the constraint back - * later means cleaning them out first. That is a correctness change, not a - * performance one, and it is reported at high. - * - `dropIndex` removes only a performance structure, and is reported at medium — - * lower on a table small enough for the difference not to matter. + * `dropUnique` and `dropPrimary` remove a **constraint**. Duplicates the database + * was rejecting a moment ago now go in, and restoring the constraint later means + * finding and resolving whatever got in meanwhile. That is a correctness change, it + * does not depend on anybody's query workload, and it is reported at high. + * + * `dropIndex` removes only a performance structure, and whether that matters depends + * on whether anything reads it — which Difflock used to be unable to say. Now it + * asks the engine, which has been counting all along: `pg_stat_user_indexes` on + * PostgreSQL, `performance_schema` on MySQL. So the finding becomes evidence: + * + * 0 reads in 274 days → low, and says so + * 2.1M reads → high, and says so + * + * The counters carry their own caveats and the rule repeats them rather than + * rounding them off. They are cumulative since the engine last reset them, so zero + * reads on a server restarted this morning means nothing; the window is quoted with + * the number. They count reads on *this* instance, so a replica serving the traffic + * is invisible from here. And where the engine will not answer at all, the rule + * falls back to what it said before: it does not know, and says that too. */ final class DropIndexRule implements MigrationRule { @@ -55,8 +66,12 @@ private function finding(MigrationContext $context, Operation $operation): Migra $name = $operation->stringArgument(0) ?? implode(', ', $operation->columns()); $constraint = $operation->method === 'dropUnique' || $operation->method === 'dropPrimary'; + $scans = $name === '' ? null : $context->database->indexScans($context->tableName(), $name); + $risk = match (true) { $constraint => RiskLevel::High, + $scans !== null && $scans === 0 => RiskLevel::Low, + $scans !== null && $scans > 0 => RiskLevel::High, $context->database->thresholds->isMedium($context->rows()) => RiskLevel::Medium, $context->rows() === null => RiskLevel::Medium, default => RiskLevel::Low, @@ -66,9 +81,7 @@ private function finding(MigrationContext $context, Operation $operation): Migra ? 'This removes a constraint the database was enforcing. Rows that would have been ' .'rejected a moment ago are now accepted, and restoring the constraint later means ' .'finding and resolving whatever got in while it was gone.' - : 'Difflock has no view of your query workload, so it cannot say whether anything depends ' - .'on this index. What it can say is that queries planned around it will be planned ' - .'differently once it is gone.'; + : $this->usage($context, $scans); $explanation .= $this->covered($context, $operation); @@ -78,11 +91,7 @@ private function finding(MigrationContext $context, Operation $operation): Migra message: 'DROP '.strtoupper(substr($operation->method, 4)).' '.$table .($name === '' ? '' : ' ('.$name.')'), explanation: $explanation, - suggestion: $constraint - ? 'Confirm nothing relies on the database enforcing uniqueness here — application-level ' - .'checks are not equivalent under concurrency.' - : 'Check the index is genuinely unused against a production-shaped workload before ' - .'dropping it; rebuilding it later on a large table is the expensive direction.', + suggestion: $this->suggestion($constraint, $scans), subject: $name === '' ? null : $name, subjectType: Subject::Index, reversible: $context->reversible(), @@ -90,6 +99,59 @@ private function finding(MigrationContext $context, Operation $operation): Migra ); } + /** + * What the engine's own counters say about this index. + * + * Every branch quotes the window alongside the number, because a scan count + * without one is uninterpretable — zero reads since a restart an hour ago is not + * evidence of anything. + */ + private function usage(MigrationContext $context, ?int $scans): string + { + if ($scans === null) { + return 'The engine would not say how often this index has been read, so Difflock cannot ' + .'tell you whether anything depends on it. What it can say is that queries planned ' + .'around it will be planned differently once it is gone.'; + } + + $window = $context->database->indexObservedDays(); + $over = $window === null + ? 'since the engine last reset its statistics' + : 'over the '.$window.' day'.($window === 1 ? '' : 's').' since the engine last reset its statistics'; + + if ($scans === 0) { + return 'The engine reports this index has been read '.($window === null ? 'no times ' : 'no times ') + .$over.'. That is the strongest evidence available that nothing needs it — with two ' + .'caveats: the counters are per instance, so a replica serving reads is invisible ' + .'from here, and a short window since a restart proves nothing.'; + } + + return 'The engine reports this index has been read '.Thresholds::format($scans).' time' + .($scans === 1 ? '' : 's').' '.$over.'. Something is using it, and those queries will be ' + .'planned differently once it is gone.'; + } + + private function suggestion(bool $constraint, ?int $scans): string + { + if ($constraint) { + return 'Confirm nothing relies on the database enforcing uniqueness here — application-level ' + .'checks are not equivalent under concurrency.'; + } + + if ($scans !== null && $scans > 0) { + return 'Find what reads it before dropping it. If the goal is to replace it with a better ' + .'index, create the replacement first and drop this one afterwards.'; + } + + if ($scans === 0) { + return 'Check the window is long enough to be meaningful, and that no replica or reporting ' + .'database relies on it, then this looks safe to drop.'; + } + + return 'Check the index is genuinely unused against a production-shaped workload before ' + .'dropping it; rebuilding it later on a large table is the expensive direction.'; + } + /** What the index being dropped actually covers, when the live schema can say. */ private function covered(MigrationContext $context, Operation $operation): string { diff --git a/src/Migration/Rules/RedundantIndexRule.php b/src/Migration/Rules/RedundantIndexRule.php new file mode 100644 index 0000000..4c6b929 --- /dev/null +++ b/src/Migration/Rules/RedundantIndexRule.php @@ -0,0 +1,135 @@ +index('status')` in one + * migration; a year later somebody else adds `$table->index(['status', 'type'])` for + * a new query, and now the first is dead weight nobody will ever think to look for. + * + * ## Where the rule stops + * + * Only the *leading prefix* case is reported, because only that one is certain. An + * index on `(created_at, status)` does **not** make `(status)` redundant, and the + * rule does not pretend otherwise. Partial indexes, expression indexes and differing + * access methods are left alone entirely — a GIN index and a B-tree on the same + * column are not substitutes, and Difflock cannot always tell them apart from the + * schema alone. + * + * Reported at low. Nothing breaks; it is waste, and waste that is cheap to fix now + * and awkward to find later. + */ +final class RedundantIndexRule implements MigrationRule +{ + public function identifier(): string + { + return 'redundant-index'; + } + + public function analyze(MigrationContext $context): array + { + $findings = []; + + foreach ($context->statement->operations as $operation) { + if ($operation->method !== 'index') { + // Only plain indexes. A unique index enforces a constraint the + // composite one does not, so it is never redundant against it. + continue; + } + + $columns = $operation->columns(); + + if ($columns === []) { + continue; + } + + $covering = $this->covering($context, $columns); + + if ($covering instanceof Index) { + $findings[] = $this->finding($context, $operation, $columns, $covering); + } + } + + return $findings; + } + + /** + * An existing index that already leads with exactly these columns. + * + * @param list $columns + */ + private function covering(MigrationContext $context, array $columns): ?Index + { + $table = $context->liveTable(); + + if (! $table instanceof Table) { + return null; + } + + foreach ($table->indexes as $index) { + if ($index->columns === $columns) { + // Same columns exactly — that is a duplicate, and `add-index` and the + // engine will both have something to say. Not this rule's business. + continue; + } + + if (count($index->columns) <= count($columns)) { + continue; + } + + if (array_slice($index->columns, 0, count($columns)) === $columns) { + return $index; + } + } + + return null; + } + + /** + * @param list $columns + */ + private function finding( + MigrationContext $context, + Operation $operation, + array $columns, + Index $covering, + ): MigrationFinding { + $wanted = '('.implode(', ', $columns).')'; + + return $context->finding( + rule: $this->identifier(), + risk: RiskLevel::Low, + message: 'REDUNDANT INDEX '.($context->tableName() ?? '').' '.$wanted, + explanation: 'The table already has `'.$covering->name.'` on ('.implode(', ', $covering->columns) + .'), and an index can be used for any leading subset of its columns — so that one already ' + .'serves every lookup this one would. The new index adds write cost on every insert, ' + .'update and delete, and disk to hold it, for no read it can satisfy that the existing ' + .'index cannot.', + suggestion: 'Drop this index from the migration and rely on `'.$covering->name.'`. If the ' + .'intent was a different access method or a partial index, say so explicitly — Difflock ' + .'compares columns and order only.', + subject: $operation->stringArgument(1) ?? implode(', ', $columns), + subjectType: Subject::Index, + reversible: $context->reversible(), + operation: $operation, + ); + } +} diff --git a/tests/Unit/IndexEvidenceTest.php b/tests/Unit/IndexEvidenceTest.php new file mode 100644 index 0000000..408d151 --- /dev/null +++ b/tests/Unit/IndexEvidenceTest.php @@ -0,0 +1,150 @@ + $tables + * @param array $scans + */ +function usageContext(string $body, array $tables, array $scans = [], ?int $days = null, array $rows = []): MigrationContext +{ + $parsed = parseUp($body, 'x'); + + return new MigrationContext( + $parsed, + $parsed->statements[0], + new DatabaseContext( + schema: new DatabaseSchema($tables, 'pgsql', 'main'), + statistics: new FixedTableStatistics($rows), + indexes: new FixedIndexStatistics($scans, $days), + ), + ); +} + +describe('drop-index with usage evidence', function (): void { + $drop = "Schema::table('orders', fn (Blueprint \$t) => \$t->dropIndex('orders_status_index'));"; + $table = fn (): Table => new Table('orders', [], [new Index('orders_status_index', ['status'])]); + + it('drops to low when the engine says nothing has ever read it', function () use ($drop, $table): void { + $findings = (new DropIndexRule)->analyze(usageContext( + $drop, [$table()], ['orders.orders_status_index' => 0], days: 274, + )); + + expect($findings[0]->risk)->toBe(RiskLevel::Low) + ->and($findings[0]->explanation)->toContain('read no times') + ->and($findings[0]->explanation)->toContain('274 days') + ->and($findings[0]->suggestion)->toContain('safe to drop'); + }); + + it('rises to high when something is reading it', function () use ($drop, $table): void { + $findings = (new DropIndexRule)->analyze(usageContext( + $drop, [$table()], ['orders.orders_status_index' => 2_100_000], days: 30, + )); + + expect($findings[0]->risk)->toBe(RiskLevel::High) + ->and($findings[0]->explanation)->toContain('2,100,000 times') + ->and($findings[0]->suggestion)->toContain('Find what reads it'); + }); + + /** + * The caveats are the reason the evidence can be trusted. A count without its + * window is uninterpretable, and a count from one instance says nothing about a + * replica. + */ + it('quotes the window and the caveats alongside the number', function () use ($drop, $table): void { + $findings = (new DropIndexRule)->analyze(usageContext( + $drop, [$table()], ['orders.orders_status_index' => 0], days: 274, + )); + + expect($findings[0]->explanation)->toContain('per instance') + ->toContain('replica') + ->toContain('short window since a restart proves nothing'); + }); + + it('falls back to saying it does not know when the engine will not answer', function () use ($drop, $table): void { + $findings = (new DropIndexRule)->analyze(usageContext($drop, [$table()])); + + expect($findings[0]->explanation)->toContain('would not say how often') + ->and($findings[0]->risk)->toBe(RiskLevel::Medium); + }); + + /** + * Usage evidence must never soften a constraint removal: dropping a unique index + * is a correctness change whatever the read counters say. + */ + it('never lets read counts soften a dropped constraint', function (): void { + $findings = (new DropIndexRule)->analyze(usageContext( + "Schema::table('users', fn (Blueprint \$t) => \$t->dropUnique('users_email_unique'));", + [new Table('users', [], [new Index('users_email_unique', ['email'], unique: true)])], + ['users.users_email_unique' => 0], + days: 900, + )); + + expect($findings[0]->risk)->toBe(RiskLevel::High) + ->and($findings[0]->explanation)->toContain('removes a constraint'); + }); +}); + +describe('redundant-index', function (): void { + it('spots an index a longer one already covers', function (): void { + $findings = (new RedundantIndexRule)->analyze(usageContext( + "Schema::table('orders', fn (Blueprint \$t) => \$t->index('status'));", + [new Table('orders', [], [new Index('orders_status_type_index', ['status', 'type'])])], + )); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->risk)->toBe(RiskLevel::Low) + ->and($findings[0]->message)->toBe('REDUNDANT INDEX orders (status)') + ->and($findings[0]->explanation)->toContain('orders_status_type_index') + ->and($findings[0]->explanation)->toContain('leading subset'); + }); + + it('says nothing when the existing index does not lead with the same columns', function (): void { + expect((new RedundantIndexRule)->analyze(usageContext( + "Schema::table('orders', fn (Blueprint \$t) => \$t->index('status'));", + [new Table('orders', [], [new Index('i', ['type', 'status'])])], + )))->toBeEmpty(); + }); + + it('says nothing about an identical index, which is a different problem', function (): void { + expect((new RedundantIndexRule)->analyze(usageContext( + "Schema::table('orders', fn (Blueprint \$t) => \$t->index('status'));", + [new Table('orders', [], [new Index('i', ['status'])])], + )))->toBeEmpty(); + }); + + it('leaves unique indexes alone, since they enforce something extra', function (): void { + expect((new RedundantIndexRule)->analyze(usageContext( + "Schema::table('orders', fn (Blueprint \$t) => \$t->unique('status'));", + [new Table('orders', [], [new Index('i', ['status', 'type'])])], + )))->toBeEmpty(); + }); + + it('says nothing when there is no live table to compare against', function (): void { + expect((new RedundantIndexRule)->analyze(usageContext( + "Schema::table('orders', fn (Blueprint \$t) => \$t->index('status'));", + [], + )))->toBeEmpty(); + }); + + it('matches a multi-column prefix too', function (): void { + $findings = (new RedundantIndexRule)->analyze(usageContext( + "Schema::table('orders', fn (Blueprint \$t) => \$t->index(['status', 'type']));", + [new Table('orders', [], [new Index('wide', ['status', 'type', 'created_at'])])], + )); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->explanation)->toContain('wide'); + }); +}); diff --git a/tests/Unit/RulesTest.php b/tests/Unit/RulesTest.php index 90a9d1f..8549fae 100644 --- a/tests/Unit/RulesTest.php +++ b/tests/Unit/RulesTest.php @@ -483,7 +483,7 @@ function run(MigrationRule $rule, MigrationContext $context): array ->and($findings[0]->explanation)->toContain('removes a constraint') ->and($findings[1]->risk)->toBe(RiskLevel::High) ->and($findings[2]->risk)->toBe(RiskLevel::Medium) - ->and($findings[2]->explanation)->toContain('no view of your query workload'); + ->and($findings[2]->explanation)->toContain('would not say how often'); }); it('is low on a small table', function (): void {