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

Added SQL indexes #124

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added SQL indexes #124

wants to merge 3 commits into from

Conversation

bohdan-vorona
Copy link
Contributor

Checked all the tables and added SQL indexes where needed.

@bohdan-vorona bohdan-vorona linked an issue Feb 8, 2023 that may be closed by this pull request
Copy link
Collaborator

@samdark samdark left a comment

Choose a reason for hiding this comment

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

Were the indexes added by looking at EXPLAIN of actual queries?

console/migrations/m230208_103100_add_sql_indexes.php Outdated Show resolved Hide resolved
@bohdan-vorona
Copy link
Contributor Author

Were the indexes added by looking at EXPLAIN of actual queries?

I executed EXPLAIN for some tables but in most cases decisions were made by obvious non-optimized tables with relation, like a table with a field customer_id.

@samdark
Copy link
Collaborator

samdark commented Feb 9, 2023

@w911 need your input.

@cgsmith
Copy link
Contributor

cgsmith commented Feb 13, 2023

@bohdan-vorona
Copy link
Contributor Author

Thanks, Chris

Is it a PK? 👉 The PK is enough!

I suppose here he meant the primary keys of the tables themselves. So, it's "the easiest one", so we can skip it)

Is it a target of a foreign key? 👉 Definetely!

Only several tables have real foreign keys. So, the fields like {alias_id/customer_id/facility_id/service_id} are "foreign keys" in our cases because they are related to other tables' field ID.

Are you using it to find rows? 👉 Definetely!

All those "foreign keys" are needed for: joining other tables, using related tables in GridViews (it means order by the fields, sort by the fields).

I also added indexes for some fields that are "important" for selecting/filtering by like {status/superuser/abbreviation}. Potentially they may be excessive for indexing and we can remove indexes for such columns.

@samdark Your thoughts about all this?

@samdark
Copy link
Collaborator

samdark commented Feb 14, 2023

Only several tables have real foreign keys.

That's not good. Also, it's better to add FK and indexes explicitly since only MySQL adds these automatically. PostgreSQL, for example, does not.

@samdark
Copy link
Collaborator

samdark commented Feb 14, 2023

I also added indexes for some fields that are "important"

  1. The importance should be measured.
  2. The problem with too many indexes (slow inserts, buffer size bloat etc.) could be solved by carefully using composite indexes.
  3. Before/after EXPLAIN should be compared.

@cebe
Copy link
Collaborator

cebe commented Feb 21, 2023

Only several tables have real foreign keys. So, the fields like {alias_id/customer_id/facility_id/service_id} are "foreign keys" in our cases because they are related to other tables' field ID.

For these, instead of adding a generic index, a foreign key should be added explicitly.

@bohdan-vorona
Copy link
Contributor Author

@samdark @cebe

For these, instead of adding a generic index, a foreign key should be added explicitly.

So, are we going to replace the added generic indexes like {alias_id/customer_id/facility_id/service_id} with real foreign keys with cascade deletion? If yes, I can do this.

@cgsmith
Copy link
Contributor

cgsmith commented Feb 21, 2023

@bohdan-vorona yes I think that is good for adding foreign keys. This is a low priority.

@cebe
Copy link
Collaborator

cebe commented Feb 21, 2023

Whether to allow cascade deletion should be decided for every foreign key. I would not allow it for all in general.

@bohdan-vorona
Copy link
Contributor Author

@bohdan-vorona yes I think that is good for adding foreign keys. This is a low priority.

Ok.

@samdark
Copy link
Collaborator

samdark commented Feb 22, 2023

For these, instead of adding a generic index, a foreign key should be added explicitly.

Not instead. In addition to. Only MySQL adds indexes automatically. Other DBMSes do not.

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 SQL indexes
4 participants