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

Add/Remove Indexes - Allocator Table #1118

Merged
merged 8 commits into from
Feb 15, 2024
Merged

Conversation

kaysiz
Copy link
Contributor

@kaysiz kaysiz commented Feb 9, 2024

Purpose

We have missing and unused indexes on allocator table.

closes: #1117

Approach

Open Questions and Pre-Merge TODOs

Learning

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)

  • Breaking change (fix or feature that would cause existing functionality to change)

Reviewer, please remember our guidelines:

  • Be humble in the language and feedback you give, ask don't tell.
  • Consider using positive language as opposed to neutral when offering feedback. This is to avoid the negative bias that can occur with neutral language appearing negative.
  • Offer suggestions on how to improve code e.g. simplification or expanding clarity.
  • Ensure you give reasons for the changes you are proposing.

@kaysiz kaysiz changed the title Add/Remove Indexex - Allocator Table Add/Remove Indexes - Allocator Table Feb 9, 2024
@kaysiz kaysiz marked this pull request as ready for review February 9, 2024 13:22
@kaysiz kaysiz requested a review from a team February 9, 2024 13:22
def change
add_index :allocator, :deleted_at, name: "index_allocator_deleted_at"
add_index :allocator, :role_name, name: "index_allocator_role_name"
add_index :allocator, :ror_id, name: "index_allocator_ror_id"
Copy link
Contributor

Choose a reason for hiding this comment

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

@kaysiz have we confirmed that the queries missing these indices are in fact using these indices in the query execution plan?

Copy link
Member

Choose a reason for hiding this comment

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

Good question Wendel!

I think if Rails is not smart enough to have done this and we have to do it manually, it can be a separate piece of work/PR and doesn't need to be a blocker to merging this.

Copy link
Contributor

@wendelfabianchinsamy wendelfabianchinsamy Feb 9, 2024

Choose a reason for hiding this comment

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

Cool cool. One thing we may notice is we may get a bunch of index scans instead of index seeks. Since the query executor will still need to go back into the table fetch relevant information (columns not included in the index). MySql will only use one index per query per table so index_allocator_deleted_at would be used or index_allocator_role_name will be used or neither if the query executor thinks it's unnecessary.

@wendelfabianchinsamy
Copy link
Contributor

@kaysiz something for us to keep in mind, adding additional indices will have two side-effects:

  1. We will increase the page size as we have to store additional indices (increased storage - probably less of an issue but I thought I'd add this as a heads up)
  2. We could have a detrimental effect on writes since each write will also have to update the indices.

Just thought I'd add these as a comment just so we could keep an eye on any write performance degradation or if we see a uptick in RDS storage usage.

@kaysiz
Copy link
Contributor Author

kaysiz commented Feb 9, 2024

@kaysiz something for us to keep in mind, adding additional indices will have two side-effects:

  1. We will increase the page size as we have to store additional indices (increased storage - probably less of an issue but I thought I'd add this as a heads up)
  2. We could have a detrimental effect on writes since each write will also have to update the indices.

Just thought I'd add these as a comment just so we could keep an eye on any write performance degradation or if we see a uptick in RDS storage usage.

@wendelfabianchinsamy We are doing more reading than writing to the table, we will monitor after deployment if there are any performance issues, but this should improve our read speed.


class RemoveGlobusUuidIndex < ActiveRecord::Migration[6.1]
def change
remove_index :allocator, column: :globus_uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we might need this index, because we are using this column in serializer at many places, you can take a look at this commit for reference eeb811c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashwinisukale unfortunately we are not using the column to fetch any data(that is either .where, find_by or order_by), inside the serialiser it is used to conditionally include the the attribute.

Copy link
Contributor

@ashwinisukale ashwinisukale left a comment

Choose a reason for hiding this comment

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

I have one comment regarding globus_uuid column index. Otherwise code looks good.

@kaysiz kaysiz requested a review from ashwinisukale February 12, 2024 20:06
@kaysiz kaysiz merged commit 7c7d715 into master Feb 15, 2024
13 checks passed
@kaysiz kaysiz deleted the performance/add-allocator-indexes branch February 15, 2024 17:26
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.

Add/Remove Indexes on Allocator table
5 participants