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

Chunking in statements for Oracle Compliance #4947

Merged
merged 1 commit into from
May 27, 2021

Conversation

miaulalala
Copy link
Contributor

@miaulalala miaulalala commented Apr 13, 2021

Oracle can't deal with more than 1000 values in an expression list for in queries.

Chunk all in queries so they'll use a max of 1000 values.

Fixes #4941

@ChristophWurst
Copy link
Member

/backport to stable1.9

@miaulalala

This comment has been minimized.

@miaulalala miaulalala marked this pull request as ready for review April 15, 2021 08:31
@miaulalala miaulalala self-assigned this Apr 15, 2021
@miaulalala miaulalala added this to the v1.10.0 milestone Apr 15, 2021
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks good, didn't test though

lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
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.

Blocking until #4932 is merged

lib/Db/MessageMapper.php Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
@miaulalala miaulalala requested a review from kesselb April 22, 2021 11:09
@miaulalala miaulalala force-pushed the fix/chunking-in-statements-for-oracle branch 2 times, most recently from 1735192 to be19540 Compare April 22, 2021 13:54
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.

Looks good. Some smaller nitpicks 🙈

lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
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.

Some more nitpicks 🙈

lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Show resolved Hide resolved
lib/Db/MessageMapper.php Show resolved Hide resolved
lib/Db/MessageMapper.php Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Show resolved Hide resolved
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good!

$qb->setParameter('ids', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
$results[] = $this->findRelatedData($this->findEntities($qb), $userId);
}
return array_merge(...$results);
Copy link
Member

Choose a reason for hiding this comment

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

we could also have used the flat map here

Signed-off-by: Anna Larch <anna@nextcloud.com>
@ChristophWurst ChristophWurst force-pushed the fix/chunking-in-statements-for-oracle branch from de88d52 to 93a9a4b Compare May 20, 2021 15:03
@ChristophWurst ChristophWurst merged commit 6810fd0 into master May 27, 2021
@ChristophWurst ChristophWurst deleted the fix/chunking-in-statements-for-oracle branch May 27, 2021 11:57
@backportbot-nextcloud
Copy link

The backport to stable1.9 failed. Please do this backport manually.

$tagsQuery->setParameter('ids', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
$queryResult = $tagsQuery->execute();

while (($row = $queryResult->fetch()) !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

this done doesn't actually work. fetch won't return any rows but always false, despite $queryResult->rowCount() returning 1. Investigating.

Copy link
Member

Choose a reason for hiding this comment

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

copy pasta error. the parameter is a string array, not an int array …

@xf-
Copy link
Contributor

xf- commented Jun 4, 2021

@ChristophWurst any plans to do a release with this fix in 1.9.
Still breaks our mail at the moment and 1.10 looks more like it will be released with NC 22 and 1.9 no open PR/Backport for 1.9.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: More than 1000 expressions in a list are not allowed on Oracle
4 participants