Skip to content
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

Check if promotions exist without extra db query #3586

Merged
merged 1 commit into from
Apr 19, 2020

Conversation

katafrakt
Copy link

@katafrakt katafrakt commented Apr 14, 2020

Description

Using @promotions.any? causes an extra db query (SELECT DISTINCT ... LIMIT 1, see below). Instead, we can force-load the dataset and check its length, because it is used few lines down (@promotions.each).

Relevant fragment from logs:

(625.5ms)  SELECT COUNT(DISTINCT "spree_promotions"."id") FROM "spree_promotions" LEFT OUTER JOIN "spree_promotion_codes" ON "spree_promotion_codes"."promotion_id" = "spree_promotions"."id" WHERE "spree_promotion_codes"."value" ILIKE '%f66e%'
Spree::Promotion Exists (616.3ms)  SELECT  DISTINCT "spree_promotions".* FROM "spree_promotions" LEFT OUTER JOIN "spree_promotion_codes" ON "spree_promotion_codes"."promotion_id" = "spree_promotions"."id" WHERE "spree_promotion_codes"."value" ILIKE '%f66e%' LIMIT 1 OFFSET 15
Spree::Promotion Load (1258.6ms)  SELECT  DISTINCT "spree_promotions".* FROM "spree_promotions" LEFT OUTER JOIN "spree_promotion_codes" ON "spree_promotion_codes"."promotion_id" = "spree_promotions"."id" WHERE "spree_promotion_codes"."value" ILIKE '%f66e%' ORDER BY "spree_promotions"."id" DESC LIMIT 15 OFFSET 15

Here, the query in second line is caused by @promotions.any?. It disappears after applying this change.

Performance:

It can have quite significant (positive) performance impact when there is a lot of promotions and someone is searching (b-tree index does not work for ILIKE '%f66e%, so seq scan is used). Sometimes the page even times out on that.

According to my tests on local env with ~20 million of promotion codes in 2500 promotions it can save about 20-25% of time used on rendering this view.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

Using @promotions.any? causes an extra db query (SELECT DISTINCT ...
LIMIT 1). Instead, we can force-load the dataset and check its length,
because it is used few lines down (@promotions.each).

This cuts off db time of rendering the template by approximately
one third (on the dataset of few million promotion codes).
@katafrakt katafrakt marked this pull request as ready for review April 14, 2020 19:41
@kennyadsl
Copy link
Member

kennyadsl commented Apr 14, 2020

Hey @katafrakt, thanks! Have you also already tried with .exists?. It should still hit the database with an extra query but it looks like it's way more optimized for larger record sets than any?.

@katafrakt
Copy link
Author

@kennyadsl unfortunately it seems that exists? generates same db query:

(3875.5ms)  SELECT COUNT(DISTINCT "spree_promotions"."id") FROM "spree_promotions" LEFT OUTER JOIN "spree_promotion_codes" ON "spree_promotion_codes"."promotion_id" = "spree_promotions"."id" WHERE "spree_promotion_codes"."value" ILIKE '%f66d%'
Spree::Promotion Exists (3499.2ms)  SELECT  DISTINCT "spree_promotions".* FROM "spree_promotions" LEFT OUTER JOIN "spree_promotion_codes" ON "spree_promotion_codes"."promotion_id" = "spree_promotions"."id" WHERE "spree_promotion_codes"."value" ILIKE '%f66d%' LIMIT 1 OFFSET 0
Spree::Promotion Load (3484.0ms)  SELECT  DISTINCT "spree_promotions".* FROM "spree_promotions" LEFT OUTER JOIN "spree_promotion_codes" ON "spree_promotion_codes"."promotion_id" = "spree_promotions"."id" WHERE "spree_promotion_codes"."value" ILIKE '%f66d%' ORDER BY "spree_promotions"."id" DESC LIMIT 15 OFFSET 0

@kennyadsl kennyadsl added the changelog:solidus_backend Changes to the solidus_backend gem label Apr 16, 2020
@kennyadsl
Copy link
Member

You are right, I was reading that any? and exists? produce different queries but probably it doesn't happen in this context for some reason (maybe the LEFT OUTER JOIN?).

Thanks, @katafrakt!

@aldesantis aldesantis merged commit 81d3090 into solidusio:master Apr 19, 2020
@katafrakt katafrakt deleted the remove-unneded-db-query branch April 19, 2020 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants