-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Limit parameter count per query in Cache.removeChildren #29281
Conversation
Signed-off-by: Sijmen Schoon <[email protected]>
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.
Thanks 👍
The query should stay outside the loop if possible. #27187 as example.
@kesselb Ah, thanks for the example. I'll edit this to make it more alike that one |
This involved changing CacheQueryBuilder\whereParentIn to take a parameter name, renaming the function accordingly. Signed-off-by: Sijmen Schoon <[email protected]>
(Forgot to sign-off) |
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
/backport to stable22 |
/backport to stable21 |
/backport to stable20 |
/backport to stable22 |
/backport to stable21 |
/backport to stable20 |
/backport to stable22 |
The backport to stable22 failed. Please do this backport manually. |
Fixes #25732 by limiting the amount of IDs in
Cache.removeChildren
'sIN
clauses to20481000.This number was determined by the reported (but untested by me) parameter limit of mssql being 2100 (which way under the parameter limits of the other database engines), rounding it down to the next power of 2 to provide a nice safety margin.1000 was used elsewhere in the code, so that was used insteadIn limited performance testing, this did not seem to have any negative impact on performance, with PostgreSQL benchmarks (on my laptop on battery power) even reporting a significant positive impact.
Testing
To test this, I've first created a bunch of folders, each with a bunch of subfolders, with a single file in each of those by running the following bash command in the
data/admin/files/
directory:Now scan the directory (
php occ files:scan --all
) - this is gonna take a while, I recommend making a back-up of the database at this point to return to this state later). When this is done remove thetest
directory, then scan again.This results in a
$parentIds
element count of over 100,000, which consistently reproduces the issue.