Skip to content

Commit

Permalink
qe: fix prisma/20799 take 2
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tomhoule committed Aug 30, 2023
1 parent 0a64c82 commit 6ac2e2d
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 1 deletion.
31 changes: 30 additions & 1 deletion quaint/src/ast/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,36 @@ impl<'a> Table<'a> {
/// Add unique index definition.
pub fn add_unique_index(mut self, i: impl Into<IndexDefinition<'a>>) -> 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
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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(())
}
}

0 comments on commit 6ac2e2d

Please sign in to comment.