Skip to content

feat: faster OIDC front/back-channel logout #3937

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: faster OIDC front/back-channel logout #3937

wants to merge 1 commit into from

Conversation

alnr
Copy link
Collaborator

@alnr alnr commented Feb 5, 2025

@alnr alnr self-assigned this Feb 5, 2025
@alnr alnr requested review from aeneasr and a team as code owners February 5, 2025 22:50
@alnr alnr force-pushed the alnr/faster-oidc-logout branch 2 times, most recently from 3a5ad64 to c0b6da1 Compare February 5, 2025 23:02
@alnr alnr force-pushed the alnr/faster-oidc-logout branch from c0b6da1 to aa1ee32 Compare February 6, 2025 00:37
zepatrik
zepatrik requested changes Feb 7, 2025
View reviewed changes
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Looks very good already!

persistence/sql/persister_consent.go

q = fmt.Sprintf(`
SELECT %s FROM %s c
WHERE id IN (
Copy link
Member

Choose a reason for hiding this comment

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

I think WHERE EXISTS would be more performant here as the DB doesn't need to materialize the full sub query result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am looking at some query plans. WHERE EXISTS does some wild stuff. Might have to tinker a bit more to get this perfect. It's looking like the subselect will be easier to optimize, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to self: make sure the query plan is close to optimal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zepatrik zepatrik zepatrik requested changes

@aeneasr aeneasr Awaiting requested review from aeneasr aeneasr is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@alnr alnr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alnr @zepatrik