Skip to content

Commit b92e7be

Browse files
solracsfsolracsf
committed
perf(files): clean orphaned items without a cross-table GROUP BY join
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
1 parent 705a14c commit b92e7be

2 files changed

Lines changed: 121 additions & 44 deletions

File tree

‎apps/files/lib/BackgroundJob/DeleteOrphanedItems.php‎

Lines changed: 51 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
* Delete all share entries that have no matching entries in the file cache table.
1919
*/
2020
class DeleteOrphanedItems extends TimedJob {
21-
public const CHUNK_SIZE = 200;
21+
public const CHUNK_SIZE = 1000;
2222

2323
/**
2424
* sets the correct interval for this timed job
@@ -46,74 +46,81 @@ public function run($argument) {
4646
}
4747

4848
/**
49-
* Deleting orphaned system tag mappings
49+
* Delete mapping rows of the 'files' object type whose referenced file no
50+
* longer exists in the file cache.
5051
*
51-
* @param string $table
52-
* @param string $idCol
53-
* @param string $typeCol
52+
* The candidate ids are read from the mapping table itself in keyset-paginated
53+
* chunks and each chunk is checked against the filecache primary key. This
54+
* avoids joining the (potentially huge) mapping table against filecache with a
55+
* GROUP BY, which reads the whole mapping table - and on some databases
56+
* materialises a temp table - on every run even when there are no orphans (the
57+
* common case). It also works the same way whether or not filecache is sharded,
58+
* so a single code path covers both.
59+
*
60+
* @param string $table The mapping table to clean up
61+
* @param string $idCol The column referencing the file id
62+
* @param string $typeCol The column holding the object type
63+
* @param bool $numericId Whether $idCol is an integer column. String columns
64+
* hold the numeric file id as text; the keyset cursor is
65+
* compared (and the rows ordered) using the column's own
66+
* type so the index on $idCol stays usable on every
67+
* database instead of being defeated by an implicit cast.
5468
* @return int Number of deleted entries
5569
*/
56-
protected function cleanUp(string $table, string $idCol, string $typeCol): int {
70+
protected function cleanUp(string $table, string $idCol, string $typeCol, bool $numericId): int {
5771
$deletedEntries = 0;
5872

5973
$deleteQuery = $this->connection->getQueryBuilder();
6074
$deleteQuery->delete($table)
61-
->where($deleteQuery->expr()->eq($idCol, $deleteQuery->createParameter('objectid')));
62-
63-
if ($this->connection->getShardDefinition('filecache')) {
64-
$sourceIdChunks = $this->getItemIds($table, $idCol, $typeCol, 1000);
65-
foreach ($sourceIdChunks as $sourceIdChunk) {
66-
$deletedSources = $this->findMissingSources($sourceIdChunk);
67-
$deleteQuery->setParameter('objectid', $deletedSources, IQueryBuilder::PARAM_INT_ARRAY);
68-
$deletedEntries += $deleteQuery->executeStatement();
69-
}
70-
} else {
71-
$query = $this->connection->getQueryBuilder();
72-
$query->select('t1.' . $idCol)
73-
->from($table, 't1')
74-
->where($query->expr()->eq($typeCol, $query->expr()->literal('files')))
75-
->leftJoin('t1', 'filecache', 't2', $query->expr()->eq($query->expr()->castColumn('t1.' . $idCol, IQueryBuilder::PARAM_INT), 't2.fileid'))
76-
->andWhere($query->expr()->isNull('t2.fileid'))
77-
->groupBy('t1.' . $idCol)
78-
->setMaxResults(self::CHUNK_SIZE);
79-
80-
$deleteQuery = $this->connection->getQueryBuilder();
81-
$deleteQuery->delete($table)
82-
->where($deleteQuery->expr()->in($idCol, $deleteQuery->createParameter('objectid')));
83-
84-
$deletedInLastChunk = self::CHUNK_SIZE;
85-
while ($deletedInLastChunk === self::CHUNK_SIZE) {
86-
$chunk = $query->executeQuery()->fetchFirstColumn();
87-
$deletedInLastChunk = count($chunk);
88-
89-
$deleteQuery->setParameter('objectid', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
90-
$deletedEntries += $deleteQuery->executeStatement();
75+
->where($deleteQuery->expr()->in($idCol, $deleteQuery->createParameter('objectid')));
76+
77+
foreach ($this->getItemIds($table, $idCol, $typeCol, $numericId, self::CHUNK_SIZE) as $idChunk) {
78+
$missingSources = $this->findMissingSources($idChunk);
79+
if (count($missingSources) === 0) {
80+
continue;
9181
}
82+
83+
$deleteQuery->setParameter('objectid', $missingSources, IQueryBuilder::PARAM_INT_ARRAY);
84+
$deletedEntries += $deleteQuery->executeStatement();
9285
}
9386

9487
return $deletedEntries;
9588
}
9689

9790
/**
91+
* Yield the distinct 'files' ids of $table in keyset-paginated chunks.
92+
*
93+
* Chunks are ordered by $idCol and advanced with a `$idCol > cursor`
94+
* comparison so the scan stays on the index covering ($typeCol, $idCol). The
95+
* cursor is bound - and the rows therefore ordered and compared - using the
96+
* column's own type: an integer column numerically, a string column lexically.
97+
* Mixing the two (e.g. comparing a varchar column to an integer) would force
98+
* an implicit cast that defeats the index and makes the ordering and the
99+
* comparison disagree, which could skip chunks.
100+
*
98101
* @param string $table
99102
* @param string $idCol
100103
* @param string $typeCol
104+
* @param bool $numericId Whether $idCol is an integer column
101105
* @param int $chunkSize
102106
* @return \Iterator<int[]>
103107
* @throws \OCP\DB\Exception
104108
*/
105-
private function getItemIds(string $table, string $idCol, string $typeCol, int $chunkSize): \Iterator {
109+
private function getItemIds(string $table, string $idCol, string $typeCol, bool $numericId, int $chunkSize): \Iterator {
110+
$cursorType = $numericId ? IQueryBuilder::PARAM_INT : IQueryBuilder::PARAM_STR;
111+
106112
$query = $this->connection->getQueryBuilder();
107113
$query->select($idCol)
108114
->from($table)
109115
->where($query->expr()->eq($typeCol, $query->expr()->literal('files')))
110-
->groupBy($idCol)
111116
->andWhere($query->expr()->gt($idCol, $query->createParameter('min_id')))
117+
->groupBy($idCol)
118+
->orderBy($idCol)
112119
->setMaxResults($chunkSize);
113120

114-
$minId = 0;
121+
$minId = $numericId ? 0 : '0';
115122
while (true) {
116-
$query->setParameter('min_id', $minId);
123+
$query->setParameter('min_id', $minId, $cursorType);
117124
$rows = $query->executeQuery()->fetchFirstColumn();
118125
if (count($rows) > 0) {
119126
$minId = $rows[count($rows) - 1];
@@ -139,7 +146,7 @@ private function findMissingSources(array $ids): array {
139146
* @return int Number of deleted entries
140147
*/
141148
protected function cleanSystemTags() {
142-
$deletedEntries = $this->cleanUp('systemtag_object_mapping', 'objectid', 'objecttype');
149+
$deletedEntries = $this->cleanUp('systemtag_object_mapping', 'objectid', 'objecttype', false);
143150
$this->logger->debug("$deletedEntries orphaned system tag relations deleted", ['app' => 'DeleteOrphanedItems']);
144151
return $deletedEntries;
145152
}
@@ -150,7 +157,7 @@ protected function cleanSystemTags() {
150157
* @return int Number of deleted entries
151158
*/
152159
protected function cleanUserTags() {
153-
$deletedEntries = $this->cleanUp('vcategory_to_object', 'objid', 'type');
160+
$deletedEntries = $this->cleanUp('vcategory_to_object', 'objid', 'type', true);
154161
$this->logger->debug("$deletedEntries orphaned user tag relations deleted", ['app' => 'DeleteOrphanedItems']);
155162
return $deletedEntries;
156163
}
@@ -161,7 +168,7 @@ protected function cleanUserTags() {
161168
* @return int Number of deleted entries
162169
*/
163170
protected function cleanComments() {
164-
$deletedEntries = $this->cleanUp('comments', 'object_id', 'object_type');
171+
$deletedEntries = $this->cleanUp('comments', 'object_id', 'object_type', false);
165172
$this->logger->debug("$deletedEntries orphaned comments deleted", ['app' => 'DeleteOrphanedItems']);
166173
return $deletedEntries;
167174
}
@@ -172,7 +179,7 @@ protected function cleanComments() {
172179
* @return int Number of deleted entries
173180
*/
174181
protected function cleanCommentMarkers() {
175-
$deletedEntries = $this->cleanUp('comments_read_markers', 'object_id', 'object_type');
182+
$deletedEntries = $this->cleanUp('comments_read_markers', 'object_id', 'object_type', false);
176183
$this->logger->debug("$deletedEntries orphaned comment read marks deleted", ['app' => 'DeleteOrphanedItems']);
177184
return $deletedEntries;
178185
}

‎apps/files/tests/BackgroundJob/DeleteOrphanedItemsJobTest.php‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,35 @@ protected function getMappings(string $table): array {
5151
return $mapping;
5252
}
5353

54+
protected function createFileCacheEntry(): int {
55+
$path = 'apps/files/tests/deleteorphaneditemsjobtest-' . self::getUniqueID();
56+
$query = $this->connection->getQueryBuilder();
57+
$query->insert('filecache')
58+
->values([
59+
'storage' => $query->createNamedParameter(1337, IQueryBuilder::PARAM_INT),
60+
'path' => $query->createNamedParameter($path),
61+
'path_hash' => $query->createNamedParameter(md5($path)),
62+
])->executeStatement();
63+
return $query->getLastInsertId();
64+
}
65+
66+
protected function deleteFileCacheEntry(int $fileId): void {
67+
$query = $this->connection->getQueryBuilder();
68+
$query->delete('filecache')
69+
->where($query->expr()->eq('fileid', $query->createNamedParameter($fileId, IQueryBuilder::PARAM_INT)))
70+
->executeStatement();
71+
}
72+
73+
protected function insertSystemTagMapping(int $objectId, int $tagId): void {
74+
$query = $this->connection->getQueryBuilder();
75+
$query->insert('systemtag_object_mapping')
76+
->values([
77+
'objectid' => $query->createNamedParameter($objectId, IQueryBuilder::PARAM_INT),
78+
'objecttype' => $query->createNamedParameter('files'),
79+
'systemtagid' => $query->createNamedParameter($tagId, IQueryBuilder::PARAM_INT),
80+
])->executeStatement();
81+
}
82+
5483
/**
5584
* Test clearing orphaned system tag mappings
5685
*/
@@ -100,6 +129,47 @@ public function testClearSystemTagMappings(): void {
100129
$this->cleanMapping('systemtag_object_mapping');
101130
}
102131

132+
/**
133+
* The chunked clean-up must delete every orphaned row - including a file
134+
* referenced by several tags - while keeping all rows whose file still
135+
* exists, no matter how many tags it has (the GROUP BY/dedup case).
136+
*/
137+
public function testClearSystemTagMappingsKeepsPresentRemovesOrphans(): void {
138+
$this->cleanMapping('systemtag_object_mapping');
139+
140+
$presentA = $this->createFileCacheEntry();
141+
$presentB = $this->createFileCacheEntry();
142+
// A file id guaranteed to be absent from the cache: create a row, keep
143+
// its id, then delete it again (auto-increment ids are never reused).
144+
$orphan = $this->createFileCacheEntry();
145+
$this->deleteFileCacheEntry($orphan);
146+
147+
// Present file with two tags -> both kept.
148+
$this->insertSystemTagMapping($presentA, 1);
149+
$this->insertSystemTagMapping($presentA, 2);
150+
// Present file with one tag -> kept.
151+
$this->insertSystemTagMapping($presentB, 1);
152+
// Orphaned file with two tags -> both removed.
153+
$this->insertSystemTagMapping($orphan, 1);
154+
$this->insertSystemTagMapping($orphan, 2);
155+
156+
$this->assertCount(5, $this->getMappings('systemtag_object_mapping'));
157+
158+
$job = new DeleteOrphanedItems($this->timeFactory, $this->connection, $this->logger);
159+
self::invokePrivate($job, 'cleanSystemTags');
160+
161+
$mapping = $this->getMappings('systemtag_object_mapping');
162+
$remainingIds = array_map(static fn (array $row): int => (int)$row['objectid'], $mapping);
163+
sort($remainingIds);
164+
$expected = [$presentA, $presentA, $presentB];
165+
sort($expected);
166+
$this->assertSame($expected, $remainingIds);
167+
168+
$this->deleteFileCacheEntry($presentA);
169+
$this->deleteFileCacheEntry($presentB);
170+
$this->cleanMapping('systemtag_object_mapping');
171+
}
172+
103173
/**
104174
* Test clearing orphaned system tag mappings
105175
*/

0 commit comments

Comments
 (0)