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

qe: fix pathological behaviour with many @uniques #4179

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Conversation

tomhoule
Copy link
Contributor

@tomhoule tomhoule commented Aug 30, 2023

The quaint AST lets users keep track of unique index definitions in
tables in order to generate MERGE statements in SQL Server.

The SQL query connector diligently adds every unique index to the table
definitions in the AsTable implementation for models.

The concrete method used to add the indexes is
Table::add_unique_index(). Its space complexity is O(n!) on the
number of unique indexes on a model, which quickly leads to
high memory usage, then hanging and out-of-memory errors.

See the inline comment in the commit for more details on what happened
exactly, and how this commit fixes the problem. The included regression
test would consistently take multiple minutes, then OOM on a fast
desktop machine with 32GB of RAM before the fix.

closes prisma/prisma#20799

@tomhoule tomhoule added this to the 5.3.0 milestone Aug 30, 2023
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 30, 2023

CodSpeed Performance Report

Merging #4179 will not alter performance

Comparing fix-20799-2 (8e8ab87) with main (1c5aa64)

Summary

✅ 11 untouched benchmarks

#[connector_test]
async fn repro(runner: Runner) -> TestResult<()> {
let query = r#"
query {
Copy link
Contributor

Choose a reason for hiding this comment

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

tsk tsk tsk, indoc! plz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed ✔️

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, hi Julius 👋🏻

@tomhoule tomhoule force-pushed the fix-20799-2 branch 2 times, most recently from 6ac2e2d to 1dcdccb Compare August 30, 2023 13:18
@tomhoule tomhoule changed the title qe: fix prisma/20799 take 2 qe: fix pathological behaviour with many @Uniques Aug 30, 2023
@tomhoule tomhoule mentioned this pull request Aug 30, 2023
@tomhoule tomhoule changed the title qe: fix pathological behaviour with many @Uniques qe: fix pathological behaviour with many @uniques Aug 31, 2023
The quaint AST lets users keep track of unique index definitions in
tables in order to generate `MERGE` statements in SQL Server.

The SQL query connector diligently adds every unique index to the table
definitions in the `AsTable` implementation for models.

The concrete method used to add the indexes is
`Table::add_unique_index()`. Its space complexity is `O(n!)` on the
number of unique indexes on a model, which quickly leads to
high memory usage, then hanging and out-of-memory errors.

See the inline comment in the commit for more details on what happened
exactly, and how this commit fixes the problem. The included regression
test would consistently take multiple minutes, then OOM on a fast
desktop machine with 32GB of RAM before the fix.

closes prisma/prisma#20799
@tomhoule tomhoule force-pushed the fix-20799-2 branch 2 times, most recently from 28a34d0 to 8e8ab87 Compare August 31, 2023 11:33
@tomhoule tomhoule marked this pull request as ready for review August 31, 2023 12:18
@tomhoule tomhoule requested a review from a team as a code owner August 31, 2023 12:18
@tomhoule tomhoule merged commit c67600c into main Aug 31, 2023
64 checks passed
@tomhoule tomhoule deleted the fix-20799-2 branch August 31, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suspected memory leak in Lambda function after upgrading from 4.10.1
4 participants