fix(pricing): correct inverted partial predicate on price_list_rule index (up to 50x in B2B scenario) - #16289
Open
docloulou wants to merge 1 commit into
Conversation
…ndex "IDX_price_list_rule_price_list_id" was created by the initial 2023 migration with `WHERE deleted_at IS NOT NULL`, so it only ever indexed soft-deleted rows. PricingRepository.calculatePrices resolves active price lists with `plr.price_list_id = pl.id AND plr.deleted_at IS NULL`, which the index could never serve, leaving the planner to sequentially scan price_list_rule once per candidate price row. The PriceListRule model has always declared `where: "deleted_at IS NULL"` on this index, so the migration aligns the database with the model rather than changing it. MikroORM's differ compares index names and columns but not partial predicates, so `migration:create" cannot detect or repair the drift - the DDL is written by hand, matching existing precedent in the repo (customer/Migration20251010130829.ts, fulfillment/Migration20250120115002.ts). The replacement index is built under a scratch name and swapped in by rename so concurrent reads stay live for all but the final rename.
🦋 Changeset detectedLatest commit: 87d529c The changes in this PR will be included in the next version bump. This PR includes changesets to release 79 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
docloulou
changed the title
docloulou
marked this pull request as ready for review
August 2, 2026 21:38
|
Thanks for the contribution! Initial automated review looks good. Fixes a verified database index bug: the original migration created IDX_price_list_rule_price_list_id with an inverted predicate (IS NOT NULL), meaning only soft-deleted rows were indexed. The corrective migration uses a create-then-rename strategy to keep read stall minimal. Bug confirmed in Migration20230929122253.ts:156. Model definition was always correct. Changeset present (patch on @medusajs/pricing). All PR template sections filled. Migration follows established conventions. No security, performance, or correctness concerns. Note: no linked issue for a non-trivial bug fix; contribution guidelines expect one. Triggered by: PR marked as ready for review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What — What changes are introduced in this PR?
A migration correcting the partial predicate on
IDX_price_list_rule_price_list_id, which has indexed only soft-deleted rows since #6732:The
PriceListRulemodel declaredwhere: "deleted_at IS NULL"in that same commit and still does (models/price-list-rule.ts:14-17), so this aligns the database with the model. No model, entity, or API change.Why — Why are these changes relevant or necessary?
calculatePricesresolves applicable price lists with a correlated subquery filteringplr.deleted_at IS NULL, which anIS NOT NULLindex cannot serve. The planner sequentially scansprice_list_ruleonce per candidate price row, so cost grows with the tenant's total price-list count rather than with the page — the B2B shape of one negotiated price list per customer.It only pays off when the pricing context carries a rule attribute, the price lists actually have rules, and the rule table is large enough for the planner to switch to an index scan. Both negative cases are measured below.
How — How have these changes been implemented?
DROPthenCREATE: the name already exists, soCREATE INDEX IF NOT EXISTSalone is a no-op and cannot change a predicate. The replacement is built under a scratch name and swapped in by rename, which keepsACCESS EXCLUSIVEto the rename only — read stall 1468 ms → 68 ms at 1M rows under concurrent load. Written by hand because MikroORM's differ compares index names and columns but not partial predicates:migration:createemits byte-identical output against a broken and a corrected schema. Same form ascustomer/Migration20251010130829.ts.Testing — How have these changes been tested, or how can the reviewer test the feature?
133/133 pricing integration tests pass, including the 49
calculate-price.spec.tscases covering price-list rules and customer groups. Applied through the real migration runner:[migrator] Applied 'Migration20260802103000', resulting predicateWHERE (deleted_at IS NULL),down()round-trips, no residual drift.Benchmark — standalone,
psqlonly, no Node or Medusa app; builds a scratch database and measures the query under both predicates.===> bench-price-list-rule-index.sh
Examples
No API change. Cost of one
calculatePricescall — PostgreSQL 16.14, 20,000 price sets, 340,000 price rows, 50-variant page. Only the rule table varies:Seq Scanboth — no gainSeq→Index Scan, 54xThe broken path grows linearly with rule count while the corrected one stays flat. Small or unruled tenants gain nothing from this change.
Checklist
patchon@medusajs/pricing