-
Notifications
You must be signed in to change notification settings - Fork 35
[WIP] Add expression index for sargable queries #2151
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,10 @@ pub struct Model { | |
| pub package: Option<String>, | ||
| pub product_version_range_id: Uuid, | ||
| pub context_cpe_id: Option<Uuid>, | ||
| /// Generated column: namespace part of package (NULL if no '/' in package) | ||
| pub package_namespace: Option<String>, | ||
| /// Generated column: name part of package (everything after '/' or entire package if no '/') | ||
| pub package_name: Option<String>, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name is mandatory, isn't it? If that's the case, this should be |
||
| } | ||
|
|
||
| #[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| use sea_orm_migration::prelude::*; | ||
|
|
||
| #[derive(DeriveMigrationName)] | ||
| pub struct Migration; | ||
|
|
||
| #[async_trait::async_trait] | ||
| #[allow(deprecated)] | ||
| impl MigrationTrait for Migration { | ||
| async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> { | ||
| // Add generated columns for package_namespace and package_name | ||
| // These columns split the package field to enable indexed lookups: | ||
| // - package_namespace: NULL for packages without '/', otherwise the part before '/' | ||
| // - package_name: the part after '/' if present, otherwise the entire package value | ||
| // | ||
| // Examples: | ||
| // package = "lodash" -> namespace=NULL, name="lodash" | ||
| // package = "npmjs/lodash" -> namespace="npmjs", name="lodash" | ||
| // package = "@types/node" -> namespace="@types", name="node" | ||
| // | ||
| // This maintains compatibility with existing query patterns: | ||
| // - Match on name only: WHERE package_namespace IS NULL AND package_name = ? | ||
| // - Match on namespace/name: WHERE package_namespace = ? AND package_name = ? | ||
| manager | ||
| .get_connection() | ||
| .execute_unprepared( | ||
| "ALTER TABLE product_status \ | ||
| ADD COLUMN IF NOT EXISTS package_namespace text GENERATED ALWAYS AS (\ | ||
| CASE WHEN package LIKE '%/%' THEN split_part(package, '/', 1) ELSE NULL END\ | ||
| ) STORED", | ||
| ) | ||
| .await?; | ||
|
|
||
| manager | ||
| .get_connection() | ||
| .execute_unprepared( | ||
| "ALTER TABLE product_status \ | ||
| ADD COLUMN IF NOT EXISTS package_name text GENERATED ALWAYS AS (\ | ||
| CASE WHEN package LIKE '%/%' THEN split_part(package, '/', 2) ELSE package END\ | ||
| ) STORED", | ||
| ) | ||
| .await?; | ||
|
|
||
| // Backfill existing rows with UPDATE to trigger recalculation of generated columns | ||
| manager | ||
| .get_connection() | ||
| .execute_unprepared( | ||
| "UPDATE product_status SET package = package WHERE package_namespace IS NULL OR package_name IS NULL", | ||
| ) | ||
| .await?; | ||
|
|
||
| // CONCURRENTLY (not supported by SeaORM) to avoid blocking writes | ||
| manager | ||
| .get_connection() | ||
| .execute_unprepared( | ||
| "CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_product_status_package_lookup \ | ||
| ON product_status (package_namespace, package_name)", | ||
| ) | ||
| .await?; | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To my understanding the |
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> { | ||
| manager | ||
| .get_connection() | ||
| .execute_unprepared("DROP INDEX IF EXISTS idx_product_status_package_lookup") | ||
| .await?; | ||
|
|
||
| manager | ||
| .get_connection() | ||
| .execute_unprepared("ALTER TABLE product_status DROP COLUMN IF EXISTS package_name") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be done in seaorm. Like the others.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ctron, absolutely and it will, the reason it started as SQL code it's because CONCURRENTLY is not supported by sea-orm and using it allows writes (new data) while the index gets updated.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where in that code is .execute_unprepared("ALTER TABLE product_status DROP COLUMN IF EXISTS package_name") |
||
| .await?; | ||
|
|
||
| manager | ||
| .get_connection() | ||
| .execute_unprepared( | ||
| "ALTER TABLE product_status DROP COLUMN IF EXISTS package_namespace", | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,16 @@ impl ProductStatus { | |
| advisory_id: Uuid, | ||
| vulnerability_id: String, | ||
| ) -> product_status::ActiveModel { | ||
| let (package_namespace, package_name) = | ||
| self.package | ||
| .as_ref() | ||
| .map_or((None, None), |pkg| match pkg.split_once('/') { | ||
| Some((namespace, name)) => { | ||
| (Some(namespace.to_string()), Some(name.to_string())) | ||
| } | ||
| None => (None, Some(pkg.to_string())), | ||
| }); | ||
|
|
||
| product_status::ActiveModel { | ||
| id: Set(self.uuid(advisory_id, vulnerability_id.clone())), | ||
| advisory_id: Set(advisory_id), | ||
|
|
@@ -74,6 +84,8 @@ impl ProductStatus { | |
| package: Set(self.package), | ||
| context_cpe_id: Set(self.cpe.as_ref().map(Cpe::uuid)), | ||
| product_version_range_id: Set(self.product_version_range_id), | ||
| package_namespace: Set(package_namespace), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To my understanding this should be generated by postgres, why do we need to set it here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ctron, sure, do you have pointers ? |
||
| package_name: Set(package_name), | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't need to use those in Rust code, do we need them here?