Skip to content
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

Merged
merged 2 commits into from
Nov 1, 2021

Conversation

vijfhoek
Copy link
Contributor

@vijfhoek vijfhoek commented Oct 17, 2021

Fixes #25732 by limiting the amount of IDs in Cache.removeChildren's IN clauses to 2048 1000.

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 instead

In 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:

for i in {0..100}; do
    mkdir -p test/spam$i/{0..1000}
    touch test/spam$i/{0..1000}/file
done

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 the test directory, then scan again.

This results in a $parentIds element count of over 100,000, which consistently reproduces the issue.

Signed-off-by: Sijmen Schoon <me@sijmenschoon.nl>
Copy link
Contributor

@kesselb kesselb left a 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.

lib/private/Files/Cache/Cache.php Outdated Show resolved Hide resolved
lib/private/Files/Cache/Cache.php Outdated Show resolved Hide resolved
@vijfhoek
Copy link
Contributor Author

@kesselb Ah, thanks for the example. I'll edit this to make it more alike that one

@vijfhoek vijfhoek requested a review from kesselb October 17, 2021 18:53
This involved changing CacheQueryBuilder\whereParentIn to take a
parameter name, renaming the function accordingly.

Signed-off-by: Sijmen Schoon <me@sijmenschoon.nl>
@vijfhoek
Copy link
Contributor Author

(Forgot to sign-off)

@kesselb kesselb added 3. to review Waiting for reviews bug labels Oct 17, 2021
@nickvergessen nickvergessen requested review from a team and ArtificialOwl and removed request for a team October 21, 2021 07:09
@szaimen szaimen added this to the Nextcloud 23 milestone Oct 31, 2021
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2021
19 tasks
@skjnldsv skjnldsv merged commit 63d3931 into nextcloud:master Nov 1, 2021
@welcome
Copy link

welcome bot commented Nov 1, 2021

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

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 1, 2021
@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2021

/backport to stable22

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2021

/backport to stable21

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2021

/backport to stable20

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2021

/backport to stable22

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2021

/backport to stable21

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2021

/backport to stable20

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2021

/backport to stable22

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

php occ files:scan exceeds maximal number of parameters while deleting files with PostgreSQL
5 participants