From 96364cbae85ca197e544bbfde8c531eaac7a73b3 Mon Sep 17 00:00:00 2001 From: DariusIII Date: Tue, 5 May 2026 13:18:11 +0200 Subject: [PATCH] Update cbp creation queries and indexes --- app/Services/Binaries/BinariesService.php | 18 +- app/Services/Binaries/BinaryHandler.php | 51 ++-- app/Services/Binaries/CollectionHandler.php | 149 ++++++++--- app/Services/Binaries/MissedPartHandler.php | 33 +-- app/Services/Binaries/PartHandler.php | 41 +-- app/Services/Nzb/NzbService.php | 33 ++- ...026_05_05_123242_add_cbp_query_indexes.php | 122 +++++++++ ...4540_phase2_shrink_binaries_binaryhash.php | 143 +++++++++++ tests/Feature/BinariesQueryCountTest.php | 234 ++++++++++++++++++ 9 files changed, 721 insertions(+), 103 deletions(-) create mode 100644 database/migrations/2026_05_05_123242_add_cbp_query_indexes.php create mode 100644 database/migrations/2026_05_05_124540_phase2_shrink_binaries_binaryhash.php create mode 100644 tests/Feature/BinariesQueryCountTest.php diff --git a/app/Services/Binaries/BinariesService.php b/app/Services/Binaries/BinariesService.php index 680c5a8be..03c856aa6 100644 --- a/app/Services/Binaries/BinariesService.php +++ b/app/Services/Binaries/BinariesService.php @@ -79,7 +79,8 @@ class BinariesService $this->headerStorage = $headerStorage ?? new HeaderStorageService(config: $this->config); $this->missedPartHandler = $missedPartHandler ?? new MissedPartHandler( $this->config->partRepairLimit, - $this->config->partRepairMaxTries + $this->config->partRepairMaxTries, + $this->config->bulkSqlChunkSize ); $this->nntp = $nntp; $this->startUpdate = Carbon::now(); @@ -414,12 +415,23 @@ class BinariesService $date = 0; do { - // Try to get the article date locally first + // Try to get the article date locally first. + // + // This query joins parts -> binaries -> collections starting from + // `parts.number = ?`. The parts table has PRIMARY KEY + // (binaries_id, number), so without a separate index on `number` + // the planner can only full-scan parts. The + // `ix_parts_number` index added in the + // 2026_05_05 add_cbp_query_indexes migration makes this an index + // lookup; do not drop it without revisiting this call site. + // LIMIT 1 is fine because we just need any matching collection + // date for the article. $local = DB::select( 'SELECT c.date AS date FROM collections c INNER JOIN binaries b ON(c.id=b.collections_id) INNER JOIN parts p ON(b.id=p.binaries_id) - WHERE p.number = ?', + WHERE p.number = ? + LIMIT 1', [$currentPost] ); diff --git a/app/Services/Binaries/BinaryHandler.php b/app/Services/Binaries/BinaryHandler.php index 949c09263..749aa1d96 100644 --- a/app/Services/Binaries/BinaryHandler.php +++ b/app/Services/Binaries/BinaryHandler.php @@ -193,7 +193,15 @@ final class BinaryHandler private function bulkInsertAndResolve(array $rowsByArticleKey): array { $lookupRows = array_values($rowsByArticleKey); - $existingKeys = $this->existingBinaryKeys($lookupRows); + + // One SELECT establishes both "did this row exist?" and "what is its + // id?", instead of two identical SELECTs (existingBinaryKeys + + // resolveBinaryIds) that the previous implementation issued. + $idsByKey = $this->selectBinaryIdsByKey($lookupRows); + $existingKeys = []; + foreach ($idsByKey as $key => $_id) { + $existingKeys[$key] = true; + } if (DB::getDriverName() === 'sqlite') { $this->bulkInsertBinariesSqlite($lookupRows); @@ -201,7 +209,22 @@ final class BinaryHandler $this->bulkInsertBinariesMysql($lookupRows); } - $idsByKey = $this->resolveBinaryIds($lookupRows); + // Only re-query for rows we couldn't satisfy from the prefetch + // (i.e. those that didn't exist before the bulk INSERT). + $missingRows = []; + foreach ($lookupRows as $row) { + $key = $this->binaryLookupKey((string) $row['hash'], (int) $row['collections_id']); + if (! isset($idsByKey[$key])) { + $missingRows[] = $row; + } + } + + if ($missingRows !== []) { + foreach ($this->selectBinaryIdsByKey($missingRows) as $key => $id) { + $idsByKey[$key] = $id; + } + } + foreach ($idsByKey as $key => $id) { if (! isset($existingKeys[$key])) { $this->insertedBinaryIds[$id] = true; @@ -213,16 +236,16 @@ final class BinaryHandler /** * @param list> $rows - * @return array + * @return array id keyed by binaryLookupKey(hash, collectionId) */ - private function existingBinaryKeys(array $rows): array + private function selectBinaryIdsByKey(array $rows): array { - $keys = []; + $resolved = []; foreach ($this->selectBinaryRows($rows) as $row) { - $keys[$this->binaryLookupKey((string) $row->hashvalue, (int) $row->collections_id)] = true; + $resolved[$this->binaryLookupKey((string) $row->hashvalue, (int) $row->collections_id)] = (int) $row->id; } - return $keys; + return $resolved; } /** @param list> $rows */ @@ -274,20 +297,6 @@ final class BinaryHandler } } - /** - * @param list> $rows - * @return array - */ - private function resolveBinaryIds(array $rows): array - { - $resolved = []; - foreach ($this->selectBinaryRows($rows) as $row) { - $resolved[$this->binaryLookupKey((string) $row->hashvalue, (int) $row->collections_id)] = (int) $row->id; - } - - return $resolved; - } - /** * @param list> $rows * @return list diff --git a/app/Services/Binaries/CollectionHandler.php b/app/Services/Binaries/CollectionHandler.php index d54b620e6..7551509e0 100644 --- a/app/Services/Binaries/CollectionHandler.php +++ b/app/Services/Binaries/CollectionHandler.php @@ -39,6 +39,9 @@ final class CollectionHandler /** @var array Cached collection xrefs by collection key */ private array $existingXrefs = []; + /** @var array Cached collection IDs by collectionhash (populated by bulk prefetch) */ + private array $existingIdsByHash = []; + public function __construct( ?CollectionsCleaningService $collectionsCleaning = null, ?XrefService $xrefService = null @@ -56,6 +59,7 @@ final class CollectionHandler $this->insertedCollectionIds = []; $this->batchCollectionHashes = []; $this->existingXrefs = []; + $this->existingIdsByHash = []; } /** @@ -197,7 +201,7 @@ final class CollectionHandler return $resolved; } - $this->prefetchExistingXrefs($xrefsToPrefetch); + $this->prefetchExistingCollections($xrefsToPrefetch); foreach ($pending as $collectionKey => &$row) { $row['xref_append'] = implode(' ', $this->xrefService->diffNewTokens( $this->existingXrefs[$collectionKey], @@ -230,9 +234,14 @@ final class CollectionHandler } /** + * Prefetch existing collections in one round-trip. Populates both + * $existingXrefs (keyed by collectionKey) and $existingIdsByHash so the + * subsequent bulkInsertAndResolve() can skip its existence-check SELECT + * and only re-query for the freshly inserted rows. + * * @param array $collectionHashByKey */ - private function prefetchExistingXrefs(array $collectionHashByKey): void + private function prefetchExistingCollections(array $collectionHashByKey): void { $missing = array_filter( $collectionHashByKey, @@ -246,14 +255,24 @@ final class CollectionHandler $rows = []; foreach (array_chunk(array_values($missing), self::MAX_SQL_ROWS_PER_STATEMENT) as $chunk) { - $rows += Collection::query() + foreach (Collection::query() ->whereIn('collectionhash', $chunk) - ->pluck('xref', 'collectionhash') - ->all(); + ->select(['id', 'collectionhash', 'xref']) + ->get() as $row) { + $rows[(string) $row->collectionhash] = [ + 'id' => (int) $row->id, + 'xref' => (string) $row->xref, + ]; + } } foreach ($missing as $collectionKey => $hash) { - $this->existingXrefs[$collectionKey] = isset($rows[$hash]) ? (string) $rows[$hash] : null; + if (isset($rows[$hash])) { + $this->existingXrefs[$collectionKey] = $rows[$hash]['xref']; + $this->existingIdsByHash[$hash] = $rows[$hash]['id']; + } else { + $this->existingXrefs[$collectionKey] = null; + } } } @@ -264,7 +283,18 @@ final class CollectionHandler private function bulkInsertAndResolve(array $rowsByCollectionKey): array { $hashes = array_values(array_column($rowsByCollectionKey, 'collectionhash')); - $existingHashes = $this->existingHashes($hashes); + + // The prefetch step has already populated $existingIdsByHash, so we + // know which hashes existed before the INSERT without issuing a + // separate "existingHashes" SELECT. + $existingHashes = []; + $idsByHash = []; + foreach ($hashes as $hash) { + if (isset($this->existingIdsByHash[$hash])) { + $existingHashes[$hash] = true; + $idsByHash[$hash] = $this->existingIdsByHash[$hash]; + } + } if (DB::getDriverName() === 'sqlite') { $this->bulkInsertCollectionsSqlite($rowsByCollectionKey); @@ -272,7 +302,17 @@ final class CollectionHandler $this->bulkInsertCollectionsMysql($rowsByCollectionKey, $existingHashes); } - $idsByHash = $this->resolveIdsByHash($hashes); + // Only resolve ids for the hashes we couldn't satisfy from the + // prefetch cache (i.e. the freshly inserted rows). For chunks where + // every collection already existed this issues zero extra SELECTs. + $newHashes = array_values(array_diff(array_unique($hashes), array_keys($idsByHash))); + if ($newHashes !== []) { + foreach ($this->resolveIdsByHash($newHashes) as $hash => $id) { + $idsByHash[$hash] = $id; + $this->existingIdsByHash[$hash] = $id; + } + } + foreach ($idsByHash as $hash => $id) { if (! isset($existingHashes[$hash])) { $this->insertedCollectionIds[$id] = true; @@ -282,28 +322,6 @@ final class CollectionHandler return $idsByHash; } - /** - * @param list $hashes - * @return array - */ - private function existingHashes(array $hashes): array - { - if ($hashes === []) { - return []; - } - - $existing = []; - foreach (array_chunk($hashes, self::MAX_SQL_ROWS_PER_STATEMENT) as $chunk) { - $existing += Collection::query() - ->whereIn('collectionhash', $chunk) - ->pluck('collectionhash') - ->mapWithKeys(static fn (string $hash): array => [$hash => true]) - ->all(); - } - - return $existing; - } - /** * @param array> $rowsByCollectionKey */ @@ -355,23 +373,62 @@ final class CollectionHandler ); } + // ON DUPLICATE KEY UPDATE id = LAST_INSERT_ID(id) is the standard + // "insert or do nothing" idiom that avoids re-writing existing + // rows (and the redo/binlog churn that comes with it) while still + // letting LAST_INSERT_ID() return the existing row's id. DB::statement( 'INSERT INTO collections (subject, fromname, date, xref, groups_id, totalfiles, collectionhash, collection_regexes_id, dateadded, noise) VALUES ' .implode(',', $placeholders) - .' ON DUPLICATE KEY UPDATE dateadded = NOW()', + .' ON DUPLICATE KEY UPDATE id = LAST_INSERT_ID(id)', $bindings ); } + $this->batchAppendXrefs($rowsByCollectionKey, $existingHashes); + } + + /** + * Append xref tokens for every existing collection in a chunk in a single + * UPDATE...JOIN per sub-chunk instead of N standalone UPDATEs (one per row). + * Same UNION ALL shape as BinaryHandler::flushUpdatesMysql(). + * + * @param array> $rowsByCollectionKey + * @param array $existingHashes + */ + private function batchAppendXrefs(array $rowsByCollectionKey, array $existingHashes): void + { + $updates = []; foreach ($rowsByCollectionKey as $row) { - if ($row['xref_append'] !== '' && isset($existingHashes[$row['collectionhash']])) { - DB::update('UPDATE collections SET xref = CONCAT(xref, ?, ?) WHERE collectionhash = ?', [ - "\n", - $row['xref_append'], - $row['collectionhash'], - ]); + if (($row['xref_append'] ?? '') !== '' && isset($existingHashes[$row['collectionhash']])) { + $updates[] = [ + 'collectionhash' => $row['collectionhash'], + 'xref_append' => $row['xref_append'], + ]; } } + + if ($updates === []) { + return; + } + + foreach (array_chunk($updates, self::MAX_SQL_ROWS_PER_STATEMENT) as $chunk) { + $selects = []; + $bindings = []; + foreach ($chunk as $u) { + $selects[] = 'SELECT ? AS collectionhash, ? AS xref_append'; + $bindings[] = $u['collectionhash']; + $bindings[] = $u['xref_append']; + } + + $sql = 'UPDATE collections c INNER JOIN (' + .implode(' UNION ALL ', $selects) + .') u ON u.collectionhash = c.collectionhash ' + .'SET c.xref = CONCAT(c.xref, ?, u.xref_append)'; + $bindings[] = "\n"; + + DB::statement($sql, $bindings); + } } /** @@ -491,10 +548,13 @@ final class CollectionHandler int $regexId, string $batchNoise ): int { + // ON DUPLICATE KEY UPDATE id = LAST_INSERT_ID(id) lets LAST_INSERT_ID() + // return the existing row's id without rewriting the row (avoids the + // redo/binlog churn of `dateadded = NOW()`). $insertSql = 'INSERT INTO collections ' .'(subject, fromname, date, xref, groups_id, totalfiles, collectionhash, collection_regexes_id, dateadded, noise) ' .'VALUES (?, ?, FROM_UNIXTIME(?), ?, ?, ?, ?, ?, NOW(), ?) ' - .'ON DUPLICATE KEY UPDATE dateadded = NOW()'; + .'ON DUPLICATE KEY UPDATE id = LAST_INSERT_ID(id)'; $bindings = [ $subject, @@ -513,11 +573,18 @@ final class CollectionHandler $bindings[] = $finalXrefAppend; } - DB::statement($insertSql, $bindings); - + // affectingStatement so we can distinguish a brand-new insert + // (rowCount = 1) from a duplicate-key hit (rowCount = 0 with no xref + // append, or 2 when xref was actually appended). LAST_INSERT_ID(id) in + // the ODKU clause makes lastInsertId() return the existing row id + // even on a duplicate, so we can't rely on lastInsertId() alone. + $affected = (int) DB::affectingStatement($insertSql, $bindings); $lastId = (int) DB::connection()->getPdo()->lastInsertId(); + if ($lastId > 0) { - $this->insertedCollectionIds[$lastId] = true; + if ($affected === 1) { + $this->insertedCollectionIds[$lastId] = true; + } return $lastId; } diff --git a/app/Services/Binaries/MissedPartHandler.php b/app/Services/Binaries/MissedPartHandler.php index fff335f88..d58c48685 100644 --- a/app/Services/Binaries/MissedPartHandler.php +++ b/app/Services/Binaries/MissedPartHandler.php @@ -17,10 +17,13 @@ final class MissedPartHandler private int $partRepairMaxTries; - public function __construct(int $partRepairLimit = 15000, int $partRepairMaxTries = 3) + private int $chunkSize; + + public function __construct(int $partRepairLimit = 15000, int $partRepairMaxTries = 3, int $chunkSize = 500) { $this->partRepairLimit = $partRepairLimit; $this->partRepairMaxTries = $partRepairMaxTries; + $this->chunkSize = max(50, min(1000, $chunkSize)); } /** @@ -50,7 +53,7 @@ final class MissedPartHandler */ private function addMissingPartsSqlite(array $numbers, int $groupId): void { - foreach (array_chunk(array_unique($numbers), 1000) as $chunk) { + foreach (array_chunk(array_unique($numbers), $this->chunkSize) as $chunk) { $placeholders = []; $bindings = []; @@ -72,7 +75,7 @@ final class MissedPartHandler */ private function addMissingPartsMysql(array $numbers, int $groupId): void { - foreach (array_chunk(array_unique($numbers), 1000) as $chunk) { + foreach (array_chunk(array_unique($numbers), $this->chunkSize) as $chunk) { $placeholders = []; $bindings = []; @@ -101,12 +104,12 @@ final class MissedPartHandler } try { - DB::transaction(static function () use ($groupId, $numbers): void { - DB::table('missed_parts') - ->where('groups_id', $groupId) - ->whereIn('numberid', $numbers) - ->delete(); - }, 10); + // Single DELETE — InnoDB autocommits one statement atomically, so + // an explicit transaction here would just add round-trips. + DB::table('missed_parts') + ->where('groups_id', $groupId) + ->whereIn('numberid', $numbers) + ->delete(); } catch (\Throwable $e) { if (config('app.debug') === true) { Log::warning('removeRepairedParts failed: '.$e->getMessage()); @@ -183,11 +186,11 @@ final class MissedPartHandler */ public function cleanupExhaustedParts(int $groupId): void { - DB::transaction(function () use ($groupId): void { - DB::table('missed_parts') - ->where('groups_id', $groupId) - ->where('attempts', '>=', $this->partRepairMaxTries) - ->delete(); - }, 10); + // Single DELETE — InnoDB autocommits atomically; the explicit + // transaction wrapper would just add round-trips. + DB::table('missed_parts') + ->where('groups_id', $groupId) + ->where('attempts', '>=', $this->partRepairMaxTries) + ->delete(); } } diff --git a/app/Services/Binaries/PartHandler.php b/app/Services/Binaries/PartHandler.php index a2f8d88d8..83d6455cc 100644 --- a/app/Services/Binaries/PartHandler.php +++ b/app/Services/Binaries/PartHandler.php @@ -175,29 +175,34 @@ final class PartHandler return []; } - $keys = []; - // Group by binaries_id and use IN(...) per binary so the SELECT does - // not turn into thousands of OR clauses with two bindings each. - $numbersByBinary = []; + // Deduplicate (binaries_id, number) pairs so we don't bind the same + // tuple twice when a chunk contains repeats. + $uniquePairs = []; foreach ($parts as $part) { - $numbersByBinary[(int) $part['binaries_id']][] = $part['number']; + $bid = (int) $part['binaries_id']; + $num = (int) $part['number']; + $uniquePairs[$bid.':'.$num] = [$bid, $num]; } - foreach ($numbersByBinary as $binaryId => $numbers) { - $numbers = array_values(array_unique($numbers)); - foreach (array_chunk($numbers, self::MAX_SQL_ROWS_PER_STATEMENT) as $chunk) { - $placeholders = implode(',', array_fill(0, \count($chunk), '?')); - $bindings = $chunk; - $bindings[] = $binaryId; + $keys = []; + // Single tuple-IN per sub-chunk: (binaries_id, number) IN ((?,?),...). + // Both MySQL and SQLite (3.15+) support this row-constructor form, + // which lets one SELECT replace the previous "one SELECT per binary". + foreach (array_chunk(array_values($uniquePairs), self::MAX_SQL_ROWS_PER_STATEMENT) as $chunk) { + $tuples = implode(',', array_fill(0, \count($chunk), '(?,?)')); + $bindings = []; + foreach ($chunk as [$bid, $num]) { + $bindings[] = $bid; + $bindings[] = $num; + } - $rows = DB::select( - "SELECT binaries_id, number FROM parts WHERE number IN ({$placeholders}) AND binaries_id = ?", - $bindings - ); + $rows = DB::select( + "SELECT binaries_id, number FROM parts WHERE (binaries_id, number) IN ({$tuples})", + $bindings + ); - foreach ($rows as $row) { - $keys[$this->partKey((int) $row->binaries_id, (int) $row->number)] = true; - } + foreach ($rows as $row) { + $keys[$this->partKey((int) $row->binaries_id, (int) $row->number)] = true; } } diff --git a/app/Services/Nzb/NzbService.php b/app/Services/Nzb/NzbService.php index 432794be3..3471ae3fa 100644 --- a/app/Services/Nzb/NzbService.php +++ b/app/Services/Nzb/NzbService.php @@ -90,10 +90,33 @@ class NzbService ->select(['collections.*', DB::raw('UNIX_TIMESTAMP(collections.date) AS udate'), 'usenet_groups.name as groupname']) ->get(); - if (empty($collections)) { + if ($collections->isEmpty()) { return false; } + // Pre-load every binary and part for the release in two flat queries + // and group them in PHP, instead of issuing one binaries SELECT per + // collection and one parts SELECT per binary (1 + N + N*M queries). + $collectionIds = $collections->pluck('id')->all(); + $binariesByCollection = Binary::query() + ->whereIn('collections_id', $collectionIds) + ->orderBy('name') + ->get(['id', 'collections_id', 'name', 'totalparts']) + ->groupBy('collections_id'); + + $allBinaryIds = $binariesByCollection->flatten(1)->pluck('id')->all(); + $partsByBinary = $allBinaryIds === [] + ? collect() + : Part::query() + ->whereIn('binaries_id', $allBinaryIds) + // distinct() preserves the dedup the previous per-binary + // query had, in case two rows share (messageid, size, + // partnumber) for the same binary (defensive). + ->distinct() + ->orderBy('partnumber') + ->get(['binaries_id', 'messageid', 'size', 'partnumber']) + ->groupBy('binaries_id'); + $XMLWriter = new \XMLWriter; $XMLWriter->openMemory(); $XMLWriter->setIndent(true); @@ -120,16 +143,16 @@ class NzbService $XMLWriter->endElement(); // head foreach ($collections as $collection) { - $binaries = Binary::whereCollectionsId($collection->id)->select(['id', 'name', 'totalparts'])->orderBy('name')->get(); - if (empty($binaries)) { + $binaries = $binariesByCollection->get($collection->id); + if ($binaries === null || $binaries->isEmpty()) { return false; } $poster = $collection->fromname; foreach ($binaries as $binary) { - $parts = Part::whereBinariesId($binary->id)->distinct()->select(['messageid', 'size', 'partnumber'])->orderBy('partnumber')->get(); - if (empty($parts)) { + $parts = $partsByBinary->get($binary->id); + if ($parts === null || $parts->isEmpty()) { return false; } diff --git a/database/migrations/2026_05_05_123242_add_cbp_query_indexes.php b/database/migrations/2026_05_05_123242_add_cbp_query_indexes.php new file mode 100644 index 000000000..065b1f906 --- /dev/null +++ b/database/migrations/2026_05_05_123242_add_cbp_query_indexes.php @@ -0,0 +1,122 @@ +indexExists('parts', 'ix_parts_number')) { + Schema::table('parts', function (Blueprint $table) { + $table->index('number', 'ix_parts_number'); + }); + } + + // binaries(collections_id, binaryhash). MySQL needs an explicit prefix + // because binaryhash is BLOB; the values are UNHEX'd MD5 (16 bytes) so + // a (16) prefix indexes the full payload. SQLite has no prefix indexes + // and stores the column as a regular BLOB, so we add the composite + // without a prefix there. + if (Schema::hasTable('binaries') && ! $this->indexExists('binaries', 'ix_binaries_collection_hash')) { + if ($driver === 'sqlite') { + Schema::table('binaries', function (Blueprint $table) { + $table->index(['collections_id', 'binaryhash'], 'ix_binaries_collection_hash'); + }); + } else { + DB::statement( + 'CREATE INDEX `ix_binaries_collection_hash` ON `binaries` (`collections_id`, `binaryhash`(16))' + ); + } + } + + // The legacy ix_binaries_binaryhash(3072) prefix index is now redundant — + // every consumer pairs binaryhash with collections_id, which the new + // composite index serves directly with a much smaller key. + if (Schema::hasTable('binaries') && $this->indexExists('binaries', 'ix_binaries_binaryhash')) { + Schema::table('binaries', function (Blueprint $table) { + $table->dropIndex('ix_binaries_binaryhash'); + }); + } + + // collections(filecheck, filesize, groups_id) — covering index for the + // hot ReleaseCreationService::createReleases() filter and the + // ReleaseProcessingService stage queries that filter on filecheck. + if (Schema::hasTable('collections') && ! $this->indexExists('collections', 'ix_collections_filecheck_filesize_groups')) { + Schema::table('collections', function (Blueprint $table) { + $table->index(['filecheck', 'filesize', 'groups_id'], 'ix_collections_filecheck_filesize_groups'); + }); + } + } + + public function down(): void + { + $driver = DB::getDriverName(); + + if (Schema::hasTable('collections') && $this->indexExists('collections', 'ix_collections_filecheck_filesize_groups')) { + Schema::table('collections', function (Blueprint $table) { + $table->dropIndex('ix_collections_filecheck_filesize_groups'); + }); + } + + // Recreate the original ix_binaries_binaryhash if we dropped it. On + // MySQL it has to be a prefix index because binaryhash is a BLOB. + if (Schema::hasTable('binaries') && ! $this->indexExists('binaries', 'ix_binaries_binaryhash')) { + if ($driver === 'sqlite') { + Schema::table('binaries', function (Blueprint $table) { + $table->index('binaryhash', 'ix_binaries_binaryhash'); + }); + } else { + DB::statement('CREATE INDEX `ix_binaries_binaryhash` ON `binaries` (`binaryhash`(3072))'); + } + } + + if (Schema::hasTable('binaries') && $this->indexExists('binaries', 'ix_binaries_collection_hash')) { + Schema::table('binaries', function (Blueprint $table) { + $table->dropIndex('ix_binaries_collection_hash'); + }); + } + + if (Schema::hasTable('parts') && $this->indexExists('parts', 'ix_parts_number')) { + Schema::table('parts', function (Blueprint $table) { + $table->dropIndex('ix_parts_number'); + }); + } + } + + private function indexExists(string $table, string $indexName): bool + { + if (DB::getDriverName() === 'sqlite') { + $rows = DB::select( + "SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = ? AND name = ?", + [$table, $indexName] + ); + + return $rows !== []; + } + + $rows = DB::select("SHOW INDEX FROM `{$table}` WHERE Key_name = ?", [$indexName]); + + return $rows !== []; + } +}; diff --git a/database/migrations/2026_05_05_124540_phase2_shrink_binaries_binaryhash.php b/database/migrations/2026_05_05_124540_phase2_shrink_binaries_binaryhash.php new file mode 100644 index 000000000..f5624f8a3 --- /dev/null +++ b/database/migrations/2026_05_05_124540_phase2_shrink_binaries_binaryhash.php @@ -0,0 +1,143 @@ +columnIsBinary16('binaries', 'binaryhash')) { + // Already converted out-of-band (e.g. via pt-online-schema-change). + // Nothing to do; we just need to record the migration as run. + return; + } + + // In-place ALTER. Acceptable for small DBs only — production should + // have used pt-online-schema-change before flipping the env flag. + DB::statement( + "ALTER TABLE `binaries` MODIFY `binaryhash` BINARY(16) NOT NULL DEFAULT '\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0'" + ); + + // Recreate the composite index now that the column can be indexed in full. + if ($this->indexExists('binaries', 'ix_binaries_collection_hash')) { + DB::statement('ALTER TABLE `binaries` DROP INDEX `ix_binaries_collection_hash`'); + } + DB::statement('CREATE INDEX `ix_binaries_collection_hash` ON `binaries` (`collections_id`, `binaryhash`)'); + } + + public function down(): void + { + if (! self::isEnabled()) { + return; + } + + if (DB::getDriverName() === 'sqlite') { + return; + } + + if (! Schema::hasTable('binaries')) { + return; + } + + // Revert to the original BLOB column type. This too should be done + // via pt-online-schema-change on production-sized tables. + DB::statement( + "ALTER TABLE `binaries` MODIFY `binaryhash` BLOB NOT NULL DEFAULT '\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0\\0'" + ); + + if ($this->indexExists('binaries', 'ix_binaries_collection_hash')) { + DB::statement('ALTER TABLE `binaries` DROP INDEX `ix_binaries_collection_hash`'); + } + // BLOB requires a prefix length on MySQL. + DB::statement('CREATE INDEX `ix_binaries_collection_hash` ON `binaries` (`collections_id`, `binaryhash`(16))'); + } + + private static function isEnabled(): bool + { + // Read directly from the process environment rather than env(): this + // migration runs out-of-band from a normal request lifecycle and the + // user is meant to flip the flag immediately before invoking + // `php artisan migrate`. config() helpers would require a dedicated + // config entry just for one optional flag. + $value = getenv('RUN_PHASE2_BINARIES_HASH_SHRINK'); + + return $value === '1' || strtolower((string) $value) === 'true'; + } + + private function columnIsBinary16(string $table, string $column): bool + { + $rows = DB::select( + 'SELECT DATA_TYPE, CHARACTER_MAXIMUM_LENGTH ' + .'FROM information_schema.COLUMNS ' + .'WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ? AND COLUMN_NAME = ?', + [$table, $column] + ); + + if ($rows === []) { + return false; + } + + $row = (array) $rows[0]; + + return strtolower((string) ($row['DATA_TYPE'] ?? $row['data_type'] ?? '')) === 'binary' + && (int) ($row['CHARACTER_MAXIMUM_LENGTH'] ?? $row['character_maximum_length'] ?? 0) === 16; + } + + private function indexExists(string $table, string $indexName): bool + { + $rows = DB::select("SHOW INDEX FROM `{$table}` WHERE Key_name = ?", [$indexName]); + + return $rows !== []; + } +}; diff --git a/tests/Feature/BinariesQueryCountTest.php b/tests/Feature/BinariesQueryCountTest.php new file mode 100644 index 000000000..eb1cc5274 --- /dev/null +++ b/tests/Feature/BinariesQueryCountTest.php @@ -0,0 +1,234 @@ + 'sqlite', 'database.connections.sqlite.database' => ':memory:']); + DB::purge(); + DB::reconnect(); + + DB::statement('CREATE TABLE settings ( + section TEXT NULL, + subsection TEXT NULL, + name TEXT PRIMARY KEY, + value TEXT NULL, + hint TEXT NULL, + setting TEXT NULL + )'); + $defaults = [ + 'maxmssgs' => '20000', + 'partrepair' => '1', + 'newgroupscanmethod' => '0', + 'newgroupmsgstoscan' => '50000', + 'newgroupdaystoscan' => '3', + 'maxpartrepair' => '15000', + 'partrepairmaxtries' => '3', + ]; + foreach ($defaults as $k => $v) { + DB::table('settings')->insert(['name' => $k, 'value' => $v]); + } + + DB::statement('CREATE TABLE collections ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + subject VARCHAR(255), + fromname VARCHAR(255), + date DATETIME NULL, + xref TEXT DEFAULT "", + groups_id INT, + totalfiles INT, + collectionhash VARCHAR(40) UNIQUE, + collection_regexes_id INT, + dateadded DATETIME NULL, + noise VARCHAR(64) DEFAULT "" + )'); + + DB::statement('CREATE TABLE binaries ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + binaryhash BLOB, + name VARCHAR(255), + collections_id INT, + totalparts INT, + currentparts INT, + filenumber INT, + partsize INT, + UNIQUE(binaryhash, collections_id) + )'); + + DB::statement('CREATE TABLE parts ( + binaries_id INT, + number INT, + messageid VARCHAR(255), + partnumber INT, + size INT, + UNIQUE(binaries_id, number) + )'); + + DB::statement('CREATE TABLE missed_parts ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + numberid INT, + groups_id INT, + attempts INT DEFAULT 0, + UNIQUE(numberid, groups_id) + )'); + + DB::statement('CREATE TABLE collection_regexes ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + group_regex VARCHAR(255), + regex VARCHAR(255), + status INT DEFAULT 1, + ordinal INT DEFAULT 0 + )'); + } + + public function test_store_chunk_does_not_regress_to_n_plus_one_on_collections_or_binaries(): void + { + // CollectionsCleaningService runs a REGEXP against collection_regexes; + // SQLite has no built-in REGEXP, so this test only runs on MySQL — same + // pattern as BinariesStoreHeadersTest::test_duplicate_collection_and_binary_reuse. + if (DB::getDriverName() === 'sqlite') { + $this->markTestSkipped('Requires MySQL REGEXP function (CollectionsCleaningService).'); + } + + $harness = new TestBinariesHarness; + + // Mix of distinct subjects so we exercise multiple collections and + // binaries in a single chunk. Each pair of (subject, partNumber) + // becomes one part row. + $headers = []; + $articleNumber = 1000; + for ($i = 0; $i < 5; $i++) { + $subject = 'Subject.'.$i; + for ($p = 1; $p <= 4; $p++) { + $headers[] = $this->makeHeader($articleNumber++, $p, 4, 100, $subject); + } + } + + $counts = $this->countQueriesPerTable(static function () use ($harness, $headers): void { + $harness->publicStoreHeaders($headers); + }); + + // Ceiling rationale (per chunk, 5 distinct collections, 5 binaries): + // collections: 1 prefetch + 1 INSERT + 1 resolve-new = 3 + // binaries: 1 prefetch + 1 INSERT + 1 resolve-new = 3 + // parts: 1 INSERT (chunk fits in MAX_SQL_ROWS_PER_STATEMENT) + // Allow a small headroom so the regression test isn't brittle. + $this->assertLessThanOrEqual(8, $counts['collections'] ?? 0, + 'Collections query count should not regress to N+1; got '.($counts['collections'] ?? 0)); + $this->assertLessThanOrEqual(8, $counts['binaries'] ?? 0, + 'Binaries query count should not regress to N+1; got '.($counts['binaries'] ?? 0)); + $this->assertLessThanOrEqual(4, $counts['parts'] ?? 0, + 'Parts query count should stay bounded; got '.($counts['parts'] ?? 0)); + + // Sanity: the chunk actually persisted what we expected. + $this->assertSame(5, DB::table('collections')->count()); + $this->assertSame(5, DB::table('binaries')->count()); + $this->assertSame(20, DB::table('parts')->count()); + } + + public function test_store_chunk_re_ingest_only_runs_minimum_queries_when_everything_already_exists(): void + { + if (DB::getDriverName() === 'sqlite') { + $this->markTestSkipped('Requires MySQL REGEXP function (CollectionsCleaningService).'); + } + + $harness = new TestBinariesHarness; + + // First pass — populate. + $headers = []; + $articleNumber = 2000; + for ($i = 0; $i < 3; $i++) { + for ($p = 1; $p <= 2; $p++) { + $headers[] = $this->makeHeader($articleNumber++, $p, 2, 100, 'Reused.'.$i); + } + } + $harness->publicStoreHeaders($headers); + + // Second pass — exact same headers. The prefetch should hit every row, + // so no resolve-new SELECT should fire; existing rows skip the xref + // append UPDATE because the tokens already match. + $counts = $this->countQueriesPerTable(static function () use ($headers): void { + $freshHarness = new TestBinariesHarness; + $freshHarness->publicStoreHeaders($headers); + }); + + // With nothing new to insert, we expect at most: + // collections: 1 prefetch + 1 INSERT (no-op via ODKU id=LAST_INSERT_ID(id)) + // binaries: 1 prefetch + 1 INSERT (currentparts/partsize incremented by ODKU) + // parts: 1 INSERT IGNORE (no rows actually persist) + $this->assertLessThanOrEqual(4, $counts['collections'] ?? 0); + $this->assertLessThanOrEqual(4, $counts['binaries'] ?? 0); + $this->assertLessThanOrEqual(2, $counts['parts'] ?? 0); + } + + /** + * Run $callable while listening to executed queries, return a per-table + * SELECT/INSERT/UPDATE count keyed by lowercase table name. + * + * @return array + */ + private function countQueriesPerTable(\Closure $callable): array + { + $counts = []; + $listener = static function (QueryExecuted $event) use (&$counts): void { + // Match the first table name after FROM/INTO/UPDATE keywords. + // Strip enclosing quotes/backticks/double-quotes the grammar adds. + if (preg_match('/\b(?:from|into|update)\s+["`]?([a-zA-Z_][a-zA-Z0-9_]*)["`]?/i', $event->sql, $m)) { + $table = strtolower($m[1]); + $counts[$table] = ($counts[$table] ?? 0) + 1; + } + }; + + DB::listen($listener); + try { + $callable(); + } finally { + // Laravel does not provide a public "remove listener" for DB query + // events, but the listener captures into a local variable that + // goes out of scope when the test method returns, so the closure + // is harmless beyond this point. + } + + return $counts; + } + + /** + * @return array + */ + private function makeHeader(int $articleNumber, int $partNumber, int $totalParts, int $bytes, string $subjectBase): array + { + return [ + 'Number' => $articleNumber, + 'Subject' => $subjectBase.' ('.$partNumber.'/'.$totalParts.')', + 'From' => 'poster@example.com', + 'Date' => time(), + 'Bytes' => $bytes, + 'Message-ID' => '', + 'Xref' => 'news.example.com group:'.$articleNumber, + 'matches' => [ + 0 => $subjectBase.' ('.$partNumber.'/'.$totalParts.')', + 1 => $subjectBase, + 2 => $partNumber, + 3 => $totalParts, + ], + ]; + } +}