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

LG-15119 query optimization #11574

Merged
merged 11 commits into from
Dec 3, 2024
Merged

LG-15119 query optimization #11574

merged 11 commits into from
Dec 3, 2024

Conversation

MrNagoo
Copy link
Contributor

@MrNagoo MrNagoo commented Nov 29, 2024

Adjust queries and add index

Link to the relevant ticket:
LG-15119

🛠 Summary of changes

Noticed a timeout in the PG query. We were pulling all profiles and using ruby .uniq instead of .distinct. This needed wrapped with the read_replica time_out method.

I've also added a new IDV Facial Match level to be IAL2 opt-in and updated the queries with it.

@MrNagoo MrNagoo requested review from zachmargolis, ThatSpaceGuy and a team November 29, 2024 20:52
partner_account_status: Agreements::PartnerAccountStatus.find_by(name: 'active'),
became_partner: ..report_date,
}).
distinct
Copy link
Contributor Author

@MrNagoo MrNagoo Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has a 2x speed improvement

@facial_match_issuers ||= Profile.active.verified.facial_match.
where('verified_at <= ?', report_date.end_of_day).
distinct.
pluck(:initiating_service_provider_issuer)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now 6x faster

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice find!!

def change
add_index :profiles, %i[active idv_level verified_at], algorithm: :concurrently, name: 'index_profiles_on_active_and_idv_level_and_verified_at'
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't have too many writes. It should significantly speed up the query above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the report run fast enough without the index?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. Are you concerned about the writes for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general we should avoid creating indexes if possible. The current query that would use this runs quickly enough, and we'll hopefully be able to avoid using Postgres for it soon.

@mitchellhenke mitchellhenke force-pushed the LG-15119/query-optimization branch from ef9d53c to 49038c6 Compare December 2, 2024 19:45
require "rubygems"
require "bundler/setup"

load Gem.bin_path("rspec-core", "rspec")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

helpful to not have to type 'bundle exec'

Profile.where(active: true).where(
'verified_at <= ?',
end_date,
Profile.active.where(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated most of this file to use the available class methods.

Copy link
Contributor

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit to update the PR description prior to merge since it says it adds an index but no longer does.

🤜🤛 for both speeding up queries and making the code easier to read.

pluck(:initiating_service_provider_issuer).uniq
@facial_match_issuers ||= Reports::BaseReport.transaction_with_timeout do
Profile.active.verified.facial_match_opt_in.
where('verified_at <= ?', report_date.end_of_day).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the verified scope on the prior line redundant with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it can't be active without verified so idk why I am the way that I am.

@MrNagoo MrNagoo merged commit 2cf5663 into main Dec 3, 2024
2 checks passed
@MrNagoo MrNagoo deleted the LG-15119/query-optimization branch December 3, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants