8000 [Maintenance] Fix VersionsCleanupStackTraceDbTask (performance + logic) by BlackbitDevs · Pull Request #11855 · pimcore/pimcore · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Maintenance] Fix VersionsCleanupStackTraceDbTask (performance + logic) #11855

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

Jump to bottom
Merged
merged 7 commits into from
Apr 21, 2022
Merged

[Maintenance] Fix VersionsCleanupStackTraceDbTask (performance + logic) #11855

merged 7 commits into from
Apr 21, 2022

Conversation

BlackbitDevs
Copy link
Contributor
@BlackbitDevs BlackbitDevs commented Apr 6, 2022 •
edited
Loading

Current logic in

pimcore/lib/Maintenance/Tasks/VersionsCleanupStackTraceDbTask.php

Line 55 in 9ecae6b

$list->setOffset($i * $perIteration);
is wrong because:

  1. Imagine you have 600 matching versions
  2. Initially you select with LIMIT 0,500
  3. The stackTrace for those 500 found versions gets set to NULL
  4. When you select with LIMIT 500,500 in the next iteration, you will not find anything because after 3. there are only 100 versions left which match the condition stackTrace IS NOT NULL

But the even more severe problem is performance: We just had these processes:
Bildschirmfoto 2022-04-06 um 11 40 35
(Don't know if maintenance job locking did not work but was running with Pimcore 10.0, so this does not matter for now)

The problem is the filesort which has to be done because the database uses the index on autoSave (this condition gets added in

pimcore/models/Version/Listing/Dao.php

Lines 30 to 36 in 9ecae6b

if ($this->model->isLoadAutoSave() == false) {
if (trim($condition)) {
$condition .= ' AND autoSave = 0';
} else {
$condition = ' WHERE autoSave = 0';
}
}
)
We could create an optimized index but ordering in this case is not necessary anyway as we want to change stack trace for all found versions.

@brusch brusch added the Bug label Apr 8, 2022
@brusch brusch added this to the 10.3.5 milestone Apr 8, 2022
@brusch
Copy link
Member
brusch commented Apr 11, 2022

@BlackbitNeueMedien Thanks a lot for your PR, however I think we could even further optimize the process:

  1. we can fetch the versions in a while loop and only set the limit at the beginnen (then we also don't need the getTotalCount() and so on ...
  2. we can implement a dedicated method in the Dao for clearing the stack trace, to make it more efficient (e.g. clearStackTrace().

WDYT? 😊

@fashxp fashxp modified the milestones: 10.3.5, 10.3.6 Apr 13, 2022
@BlackbitDevs
Copy link
Contributor Author

@brusch I adjusted it to use a do-while-loop.

I did not add Version\Dao::clearStackTrace() as I do not understand how this should make clearing the stack trace more efficient. Currently the database data is always set with the setter methods of the AbstractModel objects, not directly with the Dao objects. Or have I understood you wrong?

@BlackbitDevs
Copy link
Contributor Author
BlackbitDevs commented Apr 19, 2022 •
edited
Loading

This continues to kill the database in our client's case:
Bildschirmfoto 2022-04-19 um 10 07 20
(This was before the do-while-loop but this will not make those queries any better)

The problem is that stackTrace IS NOT NULL condition cannot be indexed and autoSave=0 and date < NOW() - 7 days match a lot of versions so that the indexes are not efficient.
What do you think of
Db::get()->executeUpdate('UPDATE versions SET stackTrace=NULL WHERE date < ?', [time() - 86400 * 7]);
?
Direct update of course has same performance problem...

The only alternative I see is a "computed column" as described in https://stackoverflow.com/a/12048065

@BlackbitDevs
Copy link
Contributor Author

I added an index to versions.stackTrace with length 1 (more is not necessary to check if it is NULL or not). Now the stacktrace IS NOT NULL condition is very fast (0,0001s vs. 120s before).

brusch
brusch requested changes Apr 21, 2022
View reviewed changes
Copy link
Member
@brusch brusch left a comment

Choose a reason for hiding this comment

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

Regarding setting the stack trace to null:
What I meant is that we're currently saving the whole version again by calling $version->getDao()->save();. I'd rather implement a dedicated method, like $version->getDao()->clearStrackTrace(); that just uses:

        $this->db->update('versions', ['stackTrace' => null], ['id' => $this->model->getId()]);

to set the stack trace to null, which will be much more efficient, right?

@BlackbitDevs
Copy link
Contributor Author
BlackbitDevs commented Apr 21, 2022 •
edited
Loading

@brusch Ok, I thought it had a reason that we save the version again. But if not, then we can completely simplify this task to

Db::get()->executeUpdate('UPDATE versions SET stackTrace = NULL WHERE date < ? AND stackTrace IS NOT NULL', [time() - 86400 * 7]);

WDYT?
(I already adjusted this PR)

@brusch
Copy link
Member
brusch commented Apr 21, 2022

@BlackbitNeueMedien LGTM, can you just quickly fix the issue reported by PhpStan? Thanks again!

@BlackbitDevs
Copy link
Contributor Author
BlackbitDevs commented Apr 21, 2022 •
edited
Loading

Done. This PR is really a simplification ;-)

@brusch brusch merged commit 09abb42 into pimcore:10.3 Apr 21, 2022
@brusch
Copy link
Member
brusch commented Apr 21, 2022

Love it 💕 Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brusch brusch brusch requested changes

Assignees

@brusch brusch

Labels
Bug
Projects
None yet
Milestone
10.3.6
Development

Successfully merging this pull request may close these issues.
3 participants
@BlackbitDevs @brusch @fashxp
0