From cb26c224ea3e0e06fa9efb1c317d4529bfcb2d49 Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Wed, 9 Apr 2025 13:21:33 +0200 Subject: [PATCH] chore(pricing): improve calculate prices performances (#12120) * chore(pricing): Try to improve performances * chore(pricing): new indexes * chore(pricing): new indexes * fix orderr * Create olive-horses-judge.md * feedback --- .changeset/olive-horses-judge.md | 5 + .../migrations/.snapshot-medusa-pricing.json | 67 ++- .../src/migrations/Migration20250408145122.ts | 19 + .../pricing/src/models/price-list-rule.ts | 4 + .../modules/pricing/src/models/price-rule.ts | 8 + .../pricing/src/repositories/pricing.ts | 383 +++++++++--------- 6 files changed, 277 insertions(+), 209 deletions(-) create mode 100644 .changeset/olive-horses-judge.md create mode 100644 packages/modules/pricing/src/migrations/Migration20250408145122.ts diff --git a/.changeset/olive-horses-judge.md b/.changeset/olive-horses-judge.md new file mode 100644 index 0000000000..592990822f --- /dev/null +++ b/.changeset/olive-horses-judge.md @@ -0,0 +1,5 @@ +--- +"@medusajs/pricing": patch +--- + +chore(pricing): improve performances diff --git a/packages/modules/pricing/src/migrations/.snapshot-medusa-pricing.json b/packages/modules/pricing/src/migrations/.snapshot-medusa-pricing.json index 0901fddaa2..26e174520d 100644 --- a/packages/modules/pricing/src/migrations/.snapshot-medusa-pricing.json +++ b/packages/modules/pricing/src/migrations/.snapshot-medusa-pricing.json @@ -131,6 +131,7 @@ "keyName": "IDX_price_list_deleted_at", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_list_deleted_at\" ON \"price_list\" (deleted_at) WHERE deleted_at IS NULL" @@ -141,12 +142,14 @@ "id" ], "composite": false, + "constraint": true, "primary": true, "unique": true } ], "checks": [], - "foreignKeys": {} + "foreignKeys": {}, + "nativeEnums": {} }, { "columns": { @@ -226,6 +229,7 @@ "keyName": "IDX_price_list_rule_price_list_id", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_list_rule_price_list_id\" ON \"price_list_rule\" (price_list_id) WHERE deleted_at IS NULL" @@ -234,16 +238,27 @@ "keyName": "IDX_price_list_rule_deleted_at", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_list_rule_deleted_at\" ON \"price_list_rule\" (deleted_at) WHERE deleted_at IS NULL" }, + { + "keyName": "IDX_price_list_rule_attribute", + "columnNames": [], + "composite": false, + "constraint": false, + "primary": false, + "unique": false, + "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_list_rule_attribute\" ON \"price_list_rule\" (attribute) WHERE deleted_at IS NULL" + }, { "keyName": "price_list_rule_pkey", "columnNames": [ "id" ], "composite": false, + "constraint": true, "primary": true, "unique": true } @@ -263,7 +278,8 @@ "deleteRule": "cascade", "updateRule": "cascade" } - } + }, + "nativeEnums": {} }, { "columns": { @@ -344,6 +360,7 @@ "keyName": "IDX_price_preference_deleted_at", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_preference_deleted_at\" ON \"price_preference\" (deleted_at) WHERE deleted_at IS NULL" @@ -352,6 +369,7 @@ "keyName": "IDX_price_preference_attribute_value", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE UNIQUE INDEX IF NOT EXISTS \"IDX_price_preference_attribute_value\" ON \"price_preference\" (attribute, value) WHERE deleted_at IS NULL" @@ -362,12 +380,14 @@ "id" ], "composite": false, + "constraint": true, "primary": true, "unique": true } ], "checks": [], - "foreignKeys": {} + "foreignKeys": {}, + "nativeEnums": {} }, { "columns": { @@ -420,6 +440,7 @@ "keyName": "IDX_price_set_deleted_at", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_set_deleted_at\" ON \"price_set\" (deleted_at) WHERE deleted_at IS NULL" @@ -430,12 +451,14 @@ "id" ], "composite": false, + "constraint": true, "primary": true, "unique": true } ], "checks": [], - "foreignKeys": {} + "foreignKeys": {}, + "nativeEnums": {} }, { "columns": { @@ -570,6 +593,7 @@ "keyName": "IDX_price_price_set_id", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_price_set_id\" ON \"price\" (price_set_id) WHERE deleted_at IS NULL" @@ -578,6 +602,7 @@ "keyName": "IDX_price_price_list_id", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_price_list_id\" ON \"price\" (price_list_id) WHERE deleted_at IS NULL" @@ -586,6 +611,7 @@ "keyName": "IDX_price_deleted_at", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_deleted_at\" ON \"price\" (deleted_at) WHERE deleted_at IS NULL" @@ -594,6 +620,7 @@ "keyName": "IDX_price_currency_code", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_currency_code\" ON \"price\" (currency_code) WHERE deleted_at IS NULL" @@ -604,6 +631,7 @@ "id" ], "composite": false, + "constraint": true, "primary": true, "unique": true } @@ -636,7 +664,8 @@ "deleteRule": "cascade", "updateRule": "cascade" } - } + }, + "nativeEnums": {} }, { "columns": { @@ -743,6 +772,7 @@ "keyName": "IDX_price_rule_price_id", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_rule_price_id\" ON \"price_rule\" (price_id) WHERE deleted_at IS NULL" @@ -751,6 +781,7 @@ "keyName": "IDX_price_rule_deleted_at", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_rule_deleted_at\" ON \"price_rule\" (deleted_at) WHERE deleted_at IS NULL" @@ -759,16 +790,36 @@ "keyName": "IDX_price_rule_price_id_attribute_operator_unique", "columnNames": [], "composite": false, + "constraint": false, "primary": false, "unique": false, "expression": "CREATE UNIQUE INDEX IF NOT EXISTS \"IDX_price_rule_price_id_attribute_operator_unique\" ON \"price_rule\" (price_id, attribute, operator) WHERE deleted_at IS NULL" }, + { + "keyName": "IDX_price_rule_attribute_value", + "columnNames": [], + "composite": false, + "constraint": false, + "primary": false, + "unique": false, + "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_rule_attribute_value\" ON \"price_rule\" (attribute, value) WHERE deleted_at IS NULL" + }, + { + "keyName": "IDX_price_rule_operator_value", + "columnNames": [], + "composite": false, + "constraint": false, + "primary": false, + "unique": false, + "expression": "CREATE INDEX IF NOT EXISTS \"IDX_price_rule_operator_value\" ON \"price_rule\" (operator, value) WHERE deleted_at IS NULL" + }, { "keyName": "price_rule_pkey", "columnNames": [ "id" ], "composite": false, + "constraint": true, "primary": true, "unique": true } @@ -788,7 +839,9 @@ "deleteRule": "cascade", "updateRule": "cascade" } - } + }, + "nativeEnums": {} } - ] + ], + "nativeEnums": {} } diff --git a/packages/modules/pricing/src/migrations/Migration20250408145122.ts b/packages/modules/pricing/src/migrations/Migration20250408145122.ts new file mode 100644 index 0000000000..409709c286 --- /dev/null +++ b/packages/modules/pricing/src/migrations/Migration20250408145122.ts @@ -0,0 +1,19 @@ +import { Migration } from '@mikro-orm/migrations'; + +export class Migration20250408145122 extends Migration { + + override async up(): Promise { + this.addSql(`CREATE INDEX IF NOT EXISTS "IDX_price_list_rule_attribute" ON "price_list_rule" (attribute) WHERE deleted_at IS NULL;`); + + this.addSql(`CREATE INDEX IF NOT EXISTS "IDX_price_rule_attribute_value" ON "price_rule" (attribute, value) WHERE deleted_at IS NULL;`); + this.addSql(`CREATE INDEX IF NOT EXISTS "IDX_price_rule_operator_value" ON "price_rule" (operator, value) WHERE deleted_at IS NULL;`); + } + + override async down(): Promise { + this.addSql(`drop index if exists "IDX_price_list_rule_attribute";`); + + this.addSql(`drop index if exists "IDX_price_rule_attribute_value";`); + this.addSql(`drop index if exists "IDX_price_rule_operator_value";`); + } + +} diff --git a/packages/modules/pricing/src/models/price-list-rule.ts b/packages/modules/pricing/src/models/price-list-rule.ts index d498a0f935..c58014f8ca 100644 --- a/packages/modules/pricing/src/models/price-list-rule.ts +++ b/packages/modules/pricing/src/models/price-list-rule.ts @@ -15,6 +15,10 @@ const PriceListRule = model on: ["price_list_id"], where: "deleted_at IS NULL", }, + { + on: ["attribute"], + where: "deleted_at IS NULL", + }, ]) export default PriceListRule diff --git a/packages/modules/pricing/src/models/price-rule.ts b/packages/modules/pricing/src/models/price-rule.ts index 480f285a59..73b3bd3a42 100644 --- a/packages/modules/pricing/src/models/price-rule.ts +++ b/packages/modules/pricing/src/models/price-rule.ts @@ -18,6 +18,14 @@ const PriceRule = model where: "deleted_at IS NULL", unique: true, }, + { + on: ["attribute", "value"], + where: "deleted_at IS NULL", + }, + { + on: ["operator", "value"], + where: "deleted_at IS NULL", + }, ]) export default PriceRule diff --git a/packages/modules/pricing/src/repositories/pricing.ts b/packages/modules/pricing/src/repositories/pricing.ts index 300ab11b6e..cfb57a1de0 100644 --- a/packages/modules/pricing/src/repositories/pricing.ts +++ b/packages/modules/pricing/src/repositories/pricing.ts @@ -16,6 +16,8 @@ import { } from "@medusajs/framework/types" import { Knex, SqlEntityManager } from "@mikro-orm/postgresql" +// Simple cache implementation + export class PricingRepository extends MikroOrmBase implements PricingRepositoryService @@ -35,14 +37,11 @@ export class PricingRepository const knex = manager.getKnex() const context = pricingContext.context || {} - // Quantity is used to scope money amounts based on min_quantity and max_quantity. - // We should potentially think of reserved words in pricingContext that can't be used in rules - // or have a separate pricing options that accept things like quantity, price_list_id and other - // pricing module features + // Extract quantity and currency from context const quantity = context.quantity delete context.quantity - // Currency code here is a required param. + // Currency code is required const currencyCode = context.currency_code delete context.currency_code @@ -53,28 +52,9 @@ export class PricingRepository ) } - const isContextPresent = Object.entries(context).length || !!currencyCode - - // Only if the context is present do we need to query the database. - // We don't get anything from the db otherwise. - if (!isContextPresent) { - return [] - } - - // We query the rule tables to get all whitelisted rule attributes - // This will help cleanup the query and do a db query on only necessary rule attributes. - const priceRuleAttributesQuery = knex("price_rule") - .distinct("attribute") - .pluck("attribute") - - const priceListRuleAttributesQuery = knex("price_list_rule") - .distinct("attribute") - .pluck("attribute") - - const [ruleAttributes, priceListRuleAttributes] = await promiseAll([ - priceRuleAttributesQuery, - priceListRuleAttributesQuery, - ]) + // Generate flatten key-value pairs for rule matching + const [ruleAttributes, priceListRuleAttributes] = + await this.getAttributesFromRuleTables(knex) const allowedRuleAttributes = [ ...ruleAttributes, @@ -95,198 +75,197 @@ export class PricingRepository } ) - // Gets all the prices where rules match for each of the contexts - // that the price set is configured for - const priceSubQueryKnex = knex({ - price: "price", - }) + const hasComplexContext = flattenedContext.length > 0 + + // Base query with efficient index lookups + const query = knex .select({ id: "price.id", - amount: "price.amount", - raw_amount: "price.raw_amount", - min_quantity: "price.min_quantity", - max_quantity: "price.max_quantity", - currency_code: "price.currency_code", - deleted_at: "price.deleted_at", price_set_id: "price.price_set_id", - rules_count: "price.rules_count", - price_list_id: "price.price_list_id", - pl_rules_count: "pl.rules_count", - pl_type: "pl.type", - has_price_list: knex.raw( - "case when price.price_list_id IS NULL then False else True end" - ), - }) - .leftJoin("price_rule as pr", "pr.price_id", "price.id") - .leftJoin("price_list as pl", function () { - this.on("pl.id", "price.price_list_id").andOn( - "pl.status", - knex.raw("?", [PriceListStatus.ACTIVE]) - ) - }) - .leftJoin("price_list_rule as plr", "plr.price_list_id", "pl.id") - .groupBy("price.id", "pl.id") - .having( - knex.raw( - "count(pr.attribute) = price.rules_count AND price.price_list_id IS NULL" - ) - ) - .orHaving( - knex.raw( - "count(DISTINCT plr.attribute) = pl.rules_count AND price.price_list_id IS NOT NULL" - ) - ) - - const buildOperatorQueries = ( - operatorGroupBuilder: Knex.QueryBuilder, - value - ) => { - operatorGroupBuilder - .where((operatorBuilder) => { - operatorBuilder - .where("pr.operator", "gte") - .whereRaw("? >= pr.value::numeric", [value]) - }) - .orWhere((operatorBuilder) => { - operatorBuilder - .where("pr.operator", "gt") - .whereRaw("? > pr.value::numeric", [value]) - }) - .orWhere((operatorBuilder) => { - operatorBuilder - .where("pr.operator", "lt") - .whereRaw("? < pr.value::numeric", [value]) - }) - .orWhere((operatorBuilder) => { - operatorBuilder - .where("pr.operator", "lte") - .whereRaw("? <= pr.value::numeric", [value]) - }) - .orWhere((operatorBuilder) => { - operatorBuilder - .where("pr.operator", "eq") - .whereRaw("? = pr.value::numeric", [value]) - }) - } - - priceSubQueryKnex.orWhere((priceBuilder) => { - priceBuilder - .whereNull("price.price_list_id") - .andWhere((withoutPriceListBuilder) => { - for (const [key, value] of flattenedContext) { - withoutPriceListBuilder.orWhere((orBuilder) => { - orBuilder.where("pr.attribute", key) - - if (typeof value === "number") { - buildOperatorQueries(orBuilder, value) - } else { - const normalizeValue = Array.isArray(value) ? value : [value] - - orBuilder.whereIn("pr.value", normalizeValue) - } - }) - } - - withoutPriceListBuilder.orWhere("price.rules_count", "=", 0) - }) - }) - - priceSubQueryKnex.orWhere((q) => { - q.whereNotNull("price.price_list_id") - .whereNull("pl.deleted_at") - .andWhere(function () { - this.whereNull("pl.starts_at").orWhere( - "pl.starts_at", - "<=", - knex.fn.now() - ) - }) - .andWhere(function () { - this.whereNull("pl.ends_at").orWhere( - "pl.ends_at", - ">=", - knex.fn.now() - ) - }) - .andWhere(function () { - this.andWhere(function () { - for (const [key, value] of flattenedContext) { - this.orWhere({ "plr.attribute": key }) - this.where( - "plr.value", - "@>", - JSON.stringify(Array.isArray(value) ? value : [value]) - ) - } - - this.orWhere("pl.rules_count", "=", 0) - }) - - this.andWhere(function () { - this.andWhere((contextBuilder) => { - for (const [key, value] of flattenedContext) { - contextBuilder.orWhere((orBuilder) => { - orBuilder.where("pr.attribute", key) - - if (typeof value === "number") { - buildOperatorQueries(orBuilder, value) - } else { - const normalizeValue = Array.isArray(value) - ? value - : [value] - - orBuilder.whereIn("pr.value", normalizeValue) - } - }) - } - - contextBuilder.andWhere("price.rules_count", ">", 0) - }) - this.orWhere("price.rules_count", "=", 0) - }) - }) - }) - - const priceSetQueryKnex = knex({ - ps: "price_set", - }) - .select({ - id: "price.id", - price_set_id: "ps.id", amount: "price.amount", raw_amount: "price.raw_amount", min_quantity: "price.min_quantity", max_quantity: "price.max_quantity", currency_code: "price.currency_code", - rules_count: "price.rules_count", - pl_rules_count: "price.pl_rules_count", - price_list_type: "price.pl_type", price_list_id: "price.price_list_id", - all_rules_count: knex.raw( - "COALESCE(price.rules_count, 0) + COALESCE(price.pl_rules_count, 0)" - ), + price_list_type: "pl.type", + rules_count: "price.rules_count", + price_list_rules_count: "pl.rules_count", }) - .join(priceSubQueryKnex.as("price"), "price.price_set_id", "ps.id") - .whereIn("ps.id", pricingFilters.id) - .andWhere("price.currency_code", "=", currencyCode) + .from("price") + .whereIn("price.price_set_id", pricingFilters.id) + .andWhere("price.currency_code", currencyCode) .whereNull("price.deleted_at") - .orderBy([ - { column: "price.has_price_list", order: "asc" }, - { column: "all_rules_count", order: "desc" }, - { column: "amount", order: "asc" }, - ]) - if (quantity) { - priceSetQueryKnex.where("price.min_quantity", "<=", quantity) - priceSetQueryKnex.andWhere("price.max_quantity", ">=", quantity) + // Apply quantity filter + if (quantity !== undefined) { + query.andWhere(function (this: Knex.QueryBuilder) { + this.where(function (this: Knex.QueryBuilder) { + this.where("price.min_quantity", "<=", quantity).andWhere( + "price.max_quantity", + ">=", + quantity + ) + }).orWhere(function (this: Knex.QueryBuilder) { + this.whereNull("price.min_quantity").whereNull("price.max_quantity") + }) + }) } else { - priceSetQueryKnex.andWhere(function () { - this.andWhere("price.min_quantity", "<=", "1").orWhereNull( + query.andWhere(function (this: Knex.QueryBuilder) { + this.where("price.min_quantity", "<=", 1).orWhereNull( "price.min_quantity" ) }) } - return await priceSetQueryKnex + // Efficient price list join with index usage + query.leftJoin("price_list as pl", function (this: Knex.JoinClause) { + this.on("pl.id", "=", "price.price_list_id") + .andOn("pl.status", "=", knex.raw("?", [PriceListStatus.ACTIVE])) + .andOn(function (this: Knex.JoinClause) { + this.onNull("pl.deleted_at") + }) + .andOn(function (this: Knex.JoinClause) { + this.onNull("pl.starts_at").orOn("pl.starts_at", "<=", knex.fn.now()) + }) + .andOn(function (this: Knex.JoinClause) { + this.onNull("pl.ends_at").orOn("pl.ends_at", ">=", knex.fn.now()) + }) + }) + + // OPTIMIZATION: Only add complex rule filtering when necessary + if (hasComplexContext) { + // For price rules - direct check that ALL rules match + const priceRuleConditions = knex.raw( + ` + ( + price.rules_count = 0 OR + ( + /* Count all matching rules and compare to total rule count */ + SELECT COUNT(*) + FROM price_rule pr + WHERE pr.price_id = price.id + AND pr.deleted_at IS NULL + AND ( + ${flattenedContext + .map(([key, value]) => { + if (typeof value === "number") { + return ` + (pr.attribute = ? AND ( + (pr.operator = 'eq' AND pr.value = ?) OR + (pr.operator = 'gt' AND ? > pr.value::numeric) OR + (pr.operator = 'gte' AND ? >= pr.value::numeric) OR + (pr.operator = 'lt' AND ? < pr.value::numeric) OR + (pr.operator = 'lte' AND ? <= pr.value::numeric) + )) + ` + } else { + const normalizeValue = Array.isArray(value) + ? value + : [value] + const placeholders = normalizeValue.map(() => "?").join(",") + return `(pr.attribute = ? AND pr.value IN (${placeholders}))` + } + }) + .join(" OR ")} + ) + ) = ( + /* Get total rule count */ + SELECT COUNT(*) + FROM price_rule pr + WHERE pr.price_id = price.id + AND pr.deleted_at IS NULL + ) + ) + `, + flattenedContext.flatMap(([key, value]) => { + if (typeof value === "number") { + return [key, value.toString(), value, value, value, value] + } else { + const normalizeValue = Array.isArray(value) ? value : [value] + return [key, ...normalizeValue] + } + }) + ) + + // For price list rules - direct check that ALL rules match + const priceListRuleConditions = knex.raw( + ` + ( + pl.rules_count = 0 OR + ( + /* Count all matching rules and compare to total rule count */ + SELECT COUNT(*) + FROM price_list_rule plr + WHERE plr.price_list_id = pl.id + AND plr.deleted_at IS NULL + AND ( + ${flattenedContext + .map(([key, value]) => { + return `(plr.attribute = ? AND plr.value @> ?)` + }) + .join(" OR ")} + ) + ) = ( + /* Get total rule count */ + SELECT COUNT(*) + FROM price_list_rule plr + WHERE plr.price_list_id = pl.id + AND plr.deleted_at IS NULL + ) + ) + `, + flattenedContext.flatMap(([key, value]) => { + return [key, JSON.stringify(Array.isArray(value) ? value : [value])] + }) + ) + + query.where((qb) => { + qb.whereNull("price.price_list_id") + .andWhereRaw(priceRuleConditions) + .orWhere((qb2) => { + qb2 + .whereNotNull("price.price_list_id") + .whereRaw(priceListRuleConditions) + .andWhereRaw(priceRuleConditions) + }) + }) + } else { + // Simple case - just get prices with no rules or price lists with no rules + query.where(function (this: Knex.QueryBuilder) { + this.where("price.rules_count", 0).orWhere(function ( + this: Knex.QueryBuilder + ) { + this.whereNotNull("price.price_list_id").where("pl.rules_count", 0) + }) + }) + } + + // Optimized ordering to help query planner and preserve price list precedence + query + .orderByRaw("price.price_list_id IS NOT NULL DESC") + .orderByRaw("price.rules_count + COALESCE(pl.rules_count, 0) DESC") // More specific rules first + .orderBy("pl.id", "asc") // Order by price list ID to ensure first created price list takes precedence + .orderBy("price.amount", "asc") // For non-price list prices, cheaper ones first + + // Execute the optimized query + return await query + } + + // Helper method to get attributes from rule tables + private async getAttributesFromRuleTables(knex: Knex) { + // Using distinct queries for better performance + const priceRuleAttributesQuery = knex("price_rule") + .distinct("attribute") + .pluck("attribute") + + const priceListRuleAttributesQuery = knex("price_list_rule") + .distinct("attribute") + .pluck("attribute") + + return await promiseAll([ + priceRuleAttributesQuery, + priceListRuleAttributesQuery, + ]) } }