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.
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
Minor performance tweak for purge archive for deleted sites #14843
Conversation
@@ -116,8 +116,8 @@ public function test_purgeNoSiteArchives_PurgesAllNoSiteArchives() | |||
|
|||
//There are 5 rows for website #3. We leave the other two because they're before our purge threshold. | |||
$deletedRowCount = $this->archivePurger->purgeDeletedSiteArchives($this->january); | |||
$this->assertEquals(3 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount); | |||
self::$fixture->assertArchivesDoNotExist(array(3, 7, 10), $this->january); | |||
$this->assertEquals(5 * RawArchiveDataWithTempAndInvalidated::ROWS_PER_ARCHIVE, $deletedRowCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are now expecting more results as for deleted sites we don't need to look at ts_archived.
core/DataAccess/Model.php
Outdated
|
||
$sql = "SELECT DISTINCT idarchive FROM " . $archiveTableName . " WHERE idsite IN (".Common::getSqlStringFieldsArray($deletedSites).")"; | ||
|
||
$rows = Db::getReader()->fetchAll($sql, $deletedSites); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are hundreds or thousands of deleted sites, will this be a problem? (since we're using placeholders)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon it's unlikely this happens but will change it indeed 👍 doesn't hurt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will also apply this to the deleteArchiveIds
method that is in the same model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
Not expecting a big change in performance here. But generally it's better to have two simple queries that can be fully read from index instead of joining the tables etc. The join should be straight forward though as well because it should be also created based on the index, however the two individual queries should be still faster.
Also another tweak is that for deleted sites we don't need to look at
ts_archived
. This actually does make things quite a bit faster.