KEYCLOAK-3988: Multiple missing indexes on FKs. #3557
Conversation
|
👍 |
| @@ -22,4 +22,46 @@ | |||
| <customChange class="org.keycloak.connections.jpa.updater.liquibase.custom.MigrateUserFedToComponent"/> | |||
| </changeSet> | |||
|
|
|||
| <changeSet author="glavoie@gmail.com" id="2.4.1.index"> | |||
| <preConditions onSqlOutput="TEST" onFail="MARK_RAN"> | |||
There was a problem hiding this comment.
What's with the pre-coniditions? Would be better to just add same indexes for all dbs
There was a problem hiding this comment.
They were added after comments from @hmlnarik here: glavoie@7bb44b7
|
@hmlnarik is the preconditions for the indexes really needed? Would the db automatically create the index if one is manually created? |
|
We can leave them out for the moment and add preconditions for not adding the indices if there would be an issue with duplicate indices. I'm more inclined to add indices only for databases we have tested do not create them automatically, but I'll won't be against accepting this PR without the preconditions. The important part here is that thanks to these changes, performance on the tested databases improves, so IMHO it should certainly get in. We'll only need to put the changes into a new changeset as this did not get into 2.4.1. |
|
Ok, so I guess it makes more sense to have the pre-condition. Was looking the Liquibase docs and onSqlOutput isn't there. Also, why would it mark as ran on an error? Shouldn't it rather halt? Does MySQL and MariaDB create these indexes automatically? |
|
We started doing more performance testing for different scenarios and encountered new issues. When doing concurrent client deletion (and it would likely appear with different operations), we started to see "java.sql.SQLException: ORA-00060: deadlock detected while waiting for resource" caused by missing indices on other FKs (again on Oracle). When an index is missing, Oracle may lock a whole table rather than just the concerned rows for an operation. There are currently 81 FKs to cover. Here's some information regarding the Oracle issue: https://asktom.oracle.com/pls/asktom/f?p=100:11:0::::P11_QUESTION_ID:3932525800346405986 We also found a missing primary key in COMPOSITE_ROLE that would ensure integrity and add an implicit index. Should I modify the current PR with all the suggested changes and attach it to Keycloak's next version Liquibase changelog file? Also, should I adjust the pre-condition to create indices on all RDBMS, but exclude MySQL and MariaDB, or remove it completely? I think it is better to create "extra" indices than have large performance/database issues for FKs. The exclusion for those 2 RDBMS might also be unnecessary is the documentation mentions that they will create an index only if it is missing, and that an implicit index might be dropped if an explicit index is created on a FK: https://dev.mysql.com/doc/refman/5.7/en/create-table-foreign-keys.html
|
|
+1 To simply updating this PR with the new changes and changelog file I reckon indexes should just be created for all dbs as it seems MySQL will clean it up automatically. @hmlnarik what do you think? |
|
Yes, it seems MySQL started behaving, and the other supported databases are treating the indices in expected manner. Hence +1 on removing the constraint. |
|
Just pushed a new version of the PR with all the missing indices. Removed the condition as well. |
|
Any comment/review? |
|
PR updated. |
|
@stianst DB matrix on CI passed successfully |
There was a problem hiding this comment.
Sorry for delay, I was away. I've gone through the changes and there are quite a few indices defined that are already leftmost part of a composite index. These composite indices are used even for queries that match only on (the leftmost) part of the index so declaring them as new would be redundant. I hope I did not oversee any other duplication and after removing the indicated indices, this PR would be good to go from my point of view.
| <column name="ASSOCIATED_POLICY_ID" type="VARCHAR(36)"/> | ||
| </createIndex> | ||
| <createIndex indexName="IDX_ASSOC_POL_POL_ID" tableName="ASSOCIATED_POLICY"> | ||
| <column name="POLICY_ID" type="VARCHAR(36)"/> |
There was a problem hiding this comment.
This index is unnecessary because there is already an index for primary key on ASSOCIATED_POLICY(POLICY_ID, ASSOCIATED_POLICY_ID) from here. Please remove.
| <column name="REALM_ID" type="VARCHAR(36)"/> | ||
| </createIndex> | ||
| <createIndex indexName="IDX_CLIENT_REALM" tableName="CLIENT"> | ||
| <column name="REALM_ID" type="VARCHAR(36)"/> |
There was a problem hiding this comment.
This index is unnecessary because there is already a unique index on CLIENT(REALM_ID, NAME) from here. Please remove.
| <column name="CLIENT_TEMPLATE_ID" type="VARCHAR(36)"/> | ||
| </createIndex> | ||
| <createIndex indexName="IDX_CLIENT_ATTR_CLIENT" tableName="CLIENT_ATTRIBUTES"> | ||
| <column name="CLIENT_ID" type="VARCHAR(36)"/> |
There was a problem hiding this comment.
This index is unnecessary because there is already an index for primary key on CLIENT_ATTRIBUTES(CLIENT_ID, NAME) from here. Please remove.
| </createIndex> | ||
| <createIndex indexName="IDX_CLI_ID_PRV_MAP_ID_PRV" tableName="CLIENT_IDENTITY_PROV_MAPPING"> | ||
| <column name="IDENTITY_PROVIDER_ID" type="VARCHAR(36)"/> | ||
| </createIndex> |
There was a problem hiding this comment.
This index is unnecessary because there is already a unique index on CLIENT_IDENTITY_PROV_MAPPING(IDENTITY_PROVIDER_ID, CLIENT_ID) from here. Please remove.
| </createIndex> | ||
| <createIndex indexName="IDX_CLIENT_NODE_REG_CLIENT" tableName="CLIENT_NODE_REGISTRATIONS"> | ||
| <column name="CLIENT_ID" type="VARCHAR(36)"/> | ||
| </createIndex> |
There was a problem hiding this comment.
This index is unnecessary because there is already an index for primary key on CLIENT_NODE_REGISTRATIONS(CLIENT_ID, NAME) from here. Please remove.
| <column name="RESOURCE_SERVER_ID" type="VARCHAR(36)"/> | ||
| </createIndex> | ||
| <createIndex indexName="IDX_SCOPE_MAPPING_CLIENT" tableName="SCOPE_MAPPING"> | ||
| <column name="CLIENT_ID" type="VARCHAR(36)"/> |
There was a problem hiding this comment.
This index is unnecessary because there is already an index for primary key on SCOPE_MAPPING(CLIENT_ID, ROLE_ID) named constraint_81. Please remove.
| <column name="POLICY_ID" type="VARCHAR(36)"/> | ||
| </createIndex> | ||
| <createIndex indexName="IDX_SCOPE_POLICY_SCOPE" tableName="SCOPE_POLICY"> | ||
| <column name="SCOPE_ID" type="VARCHAR(36)"/> |
There was a problem hiding this comment.
This index is unnecessary because there is already an index for primary key on SCOPE_POLICY(SCOPE_ID, POLICY_ID) named constraint_farsrsps. Please remove.
| <column name="SCOPE_ID" type="VARCHAR(36)"/> | ||
| </createIndex> | ||
| <createIndex indexName="IDX_TEMPL_SCOPE_MAPP_TEMPL" tableName="TEMPLATE_SCOPE_MAPPING"> | ||
| <column name="TEMPLATE_ID" type="VARCHAR(36)"/> |
There was a problem hiding this comment.
This index is unnecessary because there is already an index for primary key on TEMPLATE_SCOPE_MAPPING(TEMPLATE_ID, ROLE_ID) named pk_template_scope. Please remove.
| <column name="ROLE_ID" type="VARCHAR(36)"/> | ||
| </createIndex> | ||
| <createIndex indexName="IDX_USR_FED_CFG_USER_FED_PRV" tableName="USER_FEDERATION_CONFIG"> | ||
| <column name="USER_FEDERATION_PROVIDER_ID" type="VARCHAR(36)"/> |
There was a problem hiding this comment.
This index is unnecessary because there is already an index for primary key on USER_FEDERATION_CONFIG(USER_FEDERATION_PROVIDER_ID, NAME) named constraint_f9. Please remove.
| <column name="REALM_ID" type="VARCHAR(36)"/> | ||
| </createIndex> | ||
| <createIndex indexName="IDX_USR_FD_MAP_CFG_USR_FD_MAP" tableName="USER_FEDERATION_MAPPER_CONFIG"> | ||
| <column name="USER_FEDERATION_MAPPER_ID" type="VARCHAR(36)"/> |
There was a problem hiding this comment.
This index is unnecessary because there is already an index for primary key on USER_FEDERATION_MAPPER_CONFIG(USER_FEDERATION_MAPPER_ID, NAME) named constraint_fedmapper_cfg_pm. Please remove.
| <column name="REALM_ID" type="VARCHAR(36)"/> | ||
| </createIndex> | ||
| <createIndex indexName="IDX_ID_PROV_CONF_ID_PROV" tableName="IDENTITY_PROVIDER_CONFIG"> | ||
| <column name="IDENTITY_PROVIDER_ID" type="VARCHAR(36)"/> |
There was a problem hiding this comment.
This index is unnecessary because there is already an index for primary key on IDENTITY_PROVIDER_CONFIG(IDENTITY_PROVIDER_ID, NAME) named constraint_d. Please remove.
|
Thanks for the review. I'm sorry for the amount of duplicates. Looks like the query I used totally missed the duplicates through composite indices. I'll double check everything and I'll update the PR with the changes. |
|
Updated changeset pushed! |
|
@glavoie Thanks for this contribution and sorry for the extraordinary long time it has taken us to merge it. |
During performance testing with a large number of realms, we found out that many indexes are missing for FKs, at least on an Oracle database. As indexes are not created automatically on FKs at least on Oracle, MSSQL and PostgreSQL.