-
Notifications
You must be signed in to change notification settings - Fork 1
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
[MCC-1204770] Improve accessible objects pgsql function performance when filtering #201
[MCC-1204770] Improve accessible objects pgsql function performance when filtering #201
Conversation
class UpdateAccessibleObjectsFilterPerformanceGenerator < Rails::Generators::Base | ||
source_root File.expand_path('../../../migrations', __FILE__) | ||
|
||
def generate_update_accessible_objects_filter_performance_migration |
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.
Just out of curiosity, why do we do all this migration generation manually instead of the standard rails gem_name:install:migrations
?
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.
Simple as this is the pattern the gem has been using since many years ago.
'INSERT INTO t_user_attribute_filtered_ids ' || | ||
'SELECT ua.child_id ' || | ||
'FROM t_user_attribute_ids ua ' || | ||
'JOIN policy_elements pe ' || | ||
' ON pe.id = ua.child_id ' || | ||
'WHERE %s', | ||
filter_conditions |
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.
This seems a lot better than the deleting and NOT IN clause. 👍
EXCEPTION | ||
WHEN read_only_sql_transaction THEN | ||
RETURN QUERY SELECT * FROM pm_accessible_objects_for_operations_cte(user_id, operations, field, filters); |
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.
What is this doing?
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.
This exception is raised when attempting to create a temporary table from a physical read replica. It's a weak fallback if the app is placed in read only mode.
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.
Oh, interesting. How often is pm_accessible_objects_for_operations_cte
used? Does it exist solely for RR fallback?
notes for other reviewers: https://github.com/mdsol/kjerinsky-mdsol-notes/tree/notes/2024/mcc/MCC-1189079-investigate-slow-priv (link in desc is broken) |
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.
i don't really mind (because we can't get rid of the gem anyways) but thoughts on just defining this fn in dalton itself and not messing w this gem anymore?
That would be fine. We would just need to move the specs over or create new ones to test it. Bonus would be that I could add scientist experiments. |
besides that the query LGTM, my main question was answered in your notes: i lean toward just defining the func in dalton itself and not messing around w this gem for this but i dont feel too strongly and dont wanna create extra work for you |
per discussion earlier today, seems like we go with keeping it in the gem for now. merging. |
MCC-1204770
Notes
Changes:
First statement selects by
DISTINCT
. Thechild_id
was duplicated at times in significant numbers. A very marginal difference in ms at most if at all.Altered the filter statement to be more efficient. In the reproducible example, we reduced an 8+ minute call down to 180 ms.
@mdsol/platform-scalability