From 8e8ab874bb3b1d92eee8a8c33a086d6a1c9dd8e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Houl=C3=A9?= Date: Wed, 30 Aug 2023 10:31:18 +0200 Subject: [PATCH] qe: fix pathological behaviour with many @uniques 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 https://github.com/prisma/prisma/issues/20799 --- quaint/src/ast/table.rs | 31 ++++++++- .../tests/new/regressions/mod.rs | 1 + .../tests/new/regressions/prisma_20799.rs | 65 +++++++++++++++++++ 3 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_20799.rs diff --git a/quaint/src/ast/table.rs b/quaint/src/ast/table.rs index f39937e73cd2..4b5d50161af9 100644 --- a/quaint/src/ast/table.rs +++ b/quaint/src/ast/table.rs @@ -60,7 +60,36 @@ impl<'a> Table<'a> { /// Add unique index definition. pub fn add_unique_index(mut self, i: impl Into>) -> Self { let definition = i.into(); - self.index_definitions.push(definition.set_table(self.clone())); + + // FIXME: the whole circular dependency (Table -> IndexDefinition -> Column), and cloning + // of tables inside each column. + // + // We can't clone `self` here, as that would lead to cycles. The following happened: + // `add_unique_index()` clones the table, including all previous index definitions, and + // adds the cloned table to the new index definition. On models with multiple unique + // indexes/criterias (including PKs), we repeatedly called `add_unique_index()`. Each time, + // we clone one more index, that itself contains copies of all previous indexes. Each + // column in each of these previous indexes contains a partial copy of the `Table` with + // the indexes on the table at the point of the `add_unique_index()` call that created that + // copy. + // + // If we make the simplifying assumption that all the indexes have a single column, one + // call to `add_unique_index()` would cause `(index_definitions.len() + 1)!` clones and + // allocations of `Table`s with `IndexDefinition` arrays. That quickly leads to exhausting + // available memory. With multiple columns per index, that adds a factor to each step in + // the factorial. + // + // For symptoms of the previous naive clone, see + // https://github.com/prisma/prisma/issues/20799 and the corresponding regression test in + // connector-test-kit. + let table = Table { + typ: self.typ.clone(), + alias: self.alias.clone(), + database: self.database.clone(), + index_definitions: Vec::new(), + }; + + self.index_definitions.push(definition.set_table(table)); self } diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs index bb8cfdb0ad61..f3f05163eeb2 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs @@ -18,6 +18,7 @@ mod prisma_15607; mod prisma_16760; mod prisma_17103; mod prisma_18517; +mod prisma_20799; mod prisma_5952; mod prisma_6173; mod prisma_7010; diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_20799.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_20799.rs new file mode 100644 index 000000000000..8e9c0f34ade4 --- /dev/null +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_20799.rs @@ -0,0 +1,65 @@ +use query_engine_tests::*; + +// before the fix to https://github.com/prisma/prisma/issues/20799, this test would consistently +// run for multiple minutes and crash with an OOM error on a fast desktop machine with 32GB of RAM. +#[test_suite(schema(schema), only(Sqlite))] +mod regression { + fn schema() -> String { + indoc!( + r#" +model Connection { + id Int @id @default(autoincrement()) + uid String @unique + ownerId Int + atlassianOnPremiseOAuthCredentialsId Int? @unique + bitbucketCloudOAuthCredentialsId Int? @unique + genericAppCredentialsId Int? @unique + gitlabOAuthCredentialsId Int? @unique + googleSheetsOAuthCredentialsId Int? @unique + githubOAuthCredentialsId Int? @unique + mondayOAuthCredentialsId Int? @unique + serviceNowOAuthCredentialsId Int? @unique + bitbucketOnPremiseOAuthCredentialsId Int? @unique + salesforceOAuthCredentialsId Int? @unique + tempoCloudOAuthCredentialsId Int? @unique + slackCredentialsId Int? + jsmCloudAssetsApiKeyCredentialsId Int? @unique + googleCalendarOAuthCredentialsId Int? @unique + microsoftOAuthCredentialsId Int? @unique + zoomOAuthCredentialsId Int? @unique + statuspageApiKeyCredentialsId Int? @unique + trelloApiKeyCredentialsId Int? @unique + opsgenieApiKeyCredentialsId Int? @unique + one Int? @unique + two Int? @unique + three Int? @unique + four Int? @unique + five Int? @unique + six Int? @unique + seven Int? @unique + eight Int? @unique + nine Int? @unique + ten Int? @unique +} + "# + ) + .to_owned() + } + + #[connector_test] + async fn repro(runner: Runner) -> TestResult<()> { + let query = indoc!( + r#" +query { + findManyConnection( + where: { + ownerId: 100, + }, + ) { id } +} + "# + ); + runner.query(query).await?.assert_success(); + Ok(()) + } +}