Skip to content

Optimize deletion of composite roles#45490

Merged
ahus1 merged 4 commits intokeycloak:mainfrom
ahus1:is-45065-optimize-deletion-of-composite-roles
Jan 28, 2026
Merged

Optimize deletion of composite roles#45490
ahus1 merged 4 commits intokeycloak:mainfrom
ahus1:is-45065-optimize-deletion-of-composite-roles

Conversation

@ahus1
Copy link
Copy Markdown
Member

@ahus1 ahus1 commented Jan 15, 2026 •
edited
Loading

Closes #45065

With this change and ~1000 clients, a delete is fast again, and should complete within 100ms with a local database running PostgreSQL:

$ node clientRoleDeletionDemo.js 
deleted client 69 in 0.082 seconds
deleted client 62 in 0.085 seconds
deleted client 269 in 0.077 seconds

@ahus1 ahus1 self-assigned this Jan 15, 2026
@ahus1 ahus1 force-pushed the is-45065-optimize-deletion-of-composite-roles branch from b68d242 to 30e3676 Compare January 15, 2026 22:40
@ahus1
Copy link
Copy Markdown
Member Author

ahus1 commented Jan 19, 2026 •
edited
Loading

@mposolda / @vmuzikar / @mabartos - I think the performance of this eroded in multiple changes, and the final killing bit was then the roleEntity.getParentRoles().forEach(...) which was the parent roles with all of its children.

I've now made ComposeRole its own standalone entity to avoid the previous problems, and the tests are green.

Can you please have a comment on this approach? I also had a challenge to flush the role in the right moment as Hibernate now doesn't know about the relation between CompositeRoleEntity and RoleEntity ... AFAIK I can't use RoleEntity as a primary key in ComposeRoleEntity.

Thanks!

@ahus1
Optimize deletion of composite roles
b5538e3 
Closes keycloak#45065

Signed-off-by: Alexander Schwartz <[email protected]>
@ahus1 ahus1 force-pushed the is-45065-optimize-deletion-of-composite-roles branch from 30e3676 to b5538e3 Compare January 19, 2026 15:46
@ahus1 ahus1 marked this pull request as ready for review January 19, 2026 17:58
@ahus1 ahus1 requested review from a team as code owners January 19, 2026 17:58
@stianst stianst requested a review from a team January 22, 2026 12:20
@ahus1 ahus1 force-pushed the is-45065-optimize-deletion-of-composite-roles branch from cb7d2aa to 8c583a8 Compare January 22, 2026 16:04
@ahus1 ahus1 enabled auto-merge (squash) January 22, 2026 17:34
@ahus1 ahus1 requested a review from pedroigor January 22, 2026 17:35
@ahus1 ahus1 disabled auto-merge January 22, 2026 18:16
@ahus1
More cleanup
218089f 
Signed-off-by: Alexander Schwartz <[email protected]>
@ahus1 ahus1 force-pushed the is-45065-optimize-deletion-of-composite-roles branch from 95d00c4 to 218089f Compare January 22, 2026 19:13
@ahus1 ahus1 enabled auto-merge (squash) January 22, 2026 22:10
@ahus1
Copy link
Copy Markdown
Member Author

ahus1 commented Jan 22, 2026

@pedroigor - it should now be ready for the final review. Thanks!

pedroigor
pedroigor reviewed Jan 23, 2026
View reviewed changes
Comment thread model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java
private final String parentId;
private final LazyLoader<GroupModel, MultivaluedHashMap<String, String>> attributes;
private final LazyLoader<GroupModel, Set<String>> roleMappings;
private Set<String> cachedRoleMappings = new HashSet<>();
Copy link
Copy Markdown
Contributor

@pedroigor pedroigor Jan 23, 2026

Choose a reason for hiding this comment

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

Isn't enough to rely on roleMappings? Caching is implicit when using a LazyLoader.

Copy link
Copy Markdown
Member Author

@ahus1 ahus1 Jan 23, 2026

Choose a reason for hiding this comment

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

Updated in next commit.

Comment thread model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java
}

public Set<String> getCachedRoleMappings() {
return cachedRoleMappings;
Copy link
Copy Markdown
Contributor

@pedroigor pedroigor Jan 23, 2026

Choose a reason for hiding this comment

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

Is it expected to eventually return an empty set if getRoleMappings wasn't called before?

Copy link
Copy Markdown
Member Author

@ahus1 ahus1 Jan 23, 2026

Choose a reason for hiding this comment

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

For getCachedRoleMappings() yes - this is the contract. I'll add some more javadoc for this.

Copy link
Copy Markdown
Member Author

@ahus1 ahus1 Jan 23, 2026

Choose a reason for hiding this comment

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

Updated in next commit.

@ahus1
Rework as of review
4ce9660 
Signed-off-by: Alexander Schwartz <[email protected]>
@ahus1 ahus1 requested a review from pedroigor January 23, 2026 17:34
This was referenced Jan 23, 2026
Closed
Closed
Closed
@davoustp
Copy link
Copy Markdown

davoustp commented Jan 26, 2026

Hi @ahus1, thanks for updating the initial related PR.

It looks to me that switching from an id-oriented relation table (no entity here, so Hibernate has no clue) to an entity-oriented one (there Hibernate includes it in its entities relationship graph) like you did will allow proper low-scale join selects but will actually be detrimental to the other parts of Hibernate performance, which is the dirty checking bit when looking at large scale selects: you may have a very large number of entities in Hibernate's session cache to dirty check at flush time...

My 0.02 €.

@ahus1
Copy link
Copy Markdown
Member Author

ahus1 commented Jan 26, 2026

@davoustp - thank you, noted. Yes, there will be more entities in the context when updating entities. When reading the composite roles for a roles with RoleAdapter.getChildRoles(), it should should AFAIK only load the RoleEntity.

@ahus1 ahus1 merged commit 0ddb355 into keycloak:main Jan 28, 2026
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pedroigor pedroigor pedroigor approved these changes

Assignees

@ahus1 ahus1

Labels

team/core-iam team/production-readiness

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Client deletion timeout due to large number of client roles

3 participants

@ahus1 @davoustp @pedroigor