Skip to content

feat(filecache): Scale DB query created when deleting file from filecache #54277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CarlSchwan wants to merge 2 commits into
base: master
Choose a base branch
from
+185 −19
Open

feat(filecache): Scale DB query created when deleting file from filecache #54277

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8428bd6
feat(filecache): Scale DB query created when deleting file from filec…
Aug 6, 2025
a7bb6fb
perf(metadata): Add optimized sharding for metadata deletion
Aug 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
perf(metadata): Add optimized sharding for metadata deletion 
Signed-off-by: Carl Schwan <[email protected]>
  • Loading branch information
@CarlSchwan
Carl Schwan authored and CarlSchwan committed Sep 3, 2025
commit a7bb6fb5d0948f93072b56a2ea0431e77bb97e49
8 changes: 4 additions & 4 deletions lib/private/Files/Cache/Cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,17 +623,17 @@ private function removeChildren(ICacheEntry $entry) {

$cacheEntryRemovedEvents = [];
foreach (array_combine($deletedIds, $deletedPaths) as $fileId => $filePath) {
$cacheEntryRemovedEvent = new CacheEntryRemovedEvent(
$cacheEntryRemovedEvents[] = new CacheEntryRemovedEvent(
$this->storage,
$filePath,
$fileId,
$this->getNumericStorageId()
);
$cacheEntryRemovedEvents[] = $cacheEntryRemovedEvent;
$this->eventDispatcher->dispatchTyped($cacheEntryRemovedEvent);
}
$this->eventDispatcher->dispatchTyped(new CacheEntriesRemovedEvent($cacheEntryRemovedEvents));
Copy link
Contributor

@salmart-dev salmart-dev Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: if an error happens in any app handling the previous per-item events, the core will never get a chance to handle this event leaving the data around. This is more related to handling events in code, rather than this PR itself.

Copy link
Member Author

@CarlSchwan CarlSchwan Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to send the per-item events after the grouped-item event

Copy link
Contributor

@come-nc come-nc Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is worse, it means no events gets emitted if there is an issue halway?

Copy link
Contributor

@salmart-dev salmart-dev Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CarlSchwan if you want to keep the per-item event handling after the grouped one, to avoid what Côme mentions, maybe you can wrap the first with a try-catch(Exception), so at least we can try emitting the single ones? The same could be done in reverse, I guess.

Anyways the topic of "what happens when a request crashes" during/before event handling is something that cannot be solved in this PR specifically, I just wanted to raise the point that this may lead to issues and unfortunately there is no easy way around it.


foreach ($cacheEntryRemovedEvents as $cacheEntryRemovedEvent) {
$this->eventDispatcher->dispatchTyped($cacheEntryRemovedEvent);
}
}

/**
Expand Down
4 changes: 2 additions & 2 deletions lib/private/FilesMetadata/FilesMetadataManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,9 @@ public function deleteMetadata(int $fileId): void {
}
}

public function deleteMetadataForFiles(array $fileIds): void {
public function deleteMetadataForFiles(int $storage, array $fileIds): void {
try {
$this->metadataRequestService->dropMetadataForFiles($fileIds);
$this->metadataRequestService->dropMetadataForFiles($storage, $fileIds);
} catch (Exception $e) {
$this->logger->warning('issue while deleteMetadata', ['exception' => $e, 'fileIds' => $fileIds]);
}
Expand Down
9 changes: 6 additions & 3 deletions lib/private/FilesMetadata/Listener/MetadataDelete.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,21 @@ public function handle(Event $event): void {
}

$entries = $event->getCacheEntryRemovedEvents();
$fileIds = [];
$storageToFileIds = [];

foreach ($entries as $entry) {
try {
$fileIds[] = $entry->getFileId();
$storageToFileIds[$entry->getStorageId()] ??= [];
$storageToFileIds[$entry->getStorageId()][] = $entry->getFileId();
} catch (Exception $e) {
$this->logger->warning('issue while running MetadataDelete', ['exception' => $e]);
}
}

try {
$this->filesMetadataManager->deleteMetadataForFiles($fileIds);
foreach ($storageToFileIds as $storageId => $fileIds) {
$this->filesMetadataManager->deleteMetadataForFiles($storageId, $fileIds);
}
} catch (Exception $e) {
$this->logger->warning('issue while running MetadataDelete', ['exception' => $e]);
}
Expand Down
6 changes: 3 additions & 3 deletions lib/private/FilesMetadata/Service/MetadataRequestService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,16 +147,16 @@ public function dropMetadata(int $fileId): void {

/**
* @param int[] $fileIds
* @return void
* @throws Exception
*/
public function dropMetadataForFiles(array $fileIds): void {
public function dropMetadataForFiles(int $storage, array $fileIds): void {
$chunks = array_chunk($fileIds, 1000);

foreach ($chunks as $chunk) {
$qb = $this->dbConnection->getQueryBuilder();
$qb->delete(self::TABLE_METADATA)
->where($qb->expr()->in('file_id', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)));
->where($qb->expr()->in('file_id', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)))
->hintShardKey('storage', $storage);
$qb->executeStatement();
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/public/Files/Cache/CacheEntryRemovedEvent.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
/**
* Event for when an existing entry in the cache gets removed
*
* Prefer using \c CacheEntriesRemovedEvent as it is more efficient when deleting
* Prefer using CacheEntriesRemovedEvent as it is more efficient when deleting
* multiple files at the same time.
*
* @since 21.0.0
Expand Down
3 changes: 2 additions & 1 deletion lib/public/FilesMetadata/IFilesMetadataManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,12 @@ public function deleteMetadata(int $fileId): void;
/**
* Delete metadata and its indexes of multiple file ids
*
* @param int $storage The storage id coresponding to the $fileIds
* @param array<int> $fileIds file ids
* @return void
* @since 32.0.0
*/
public function deleteMetadataForFiles(array $fileIds): void;
public function deleteMetadataForFiles(int $storage, array $fileIds): void;

/**
* generate and return a MetadataQuery to help building sql queries
Expand Down
Loading