Skip to content

Add startup check for missing database indexes#48696

Merged
ahus1 merged 2 commits intokeycloak:mainfrom
pruivo:t_48695_db_index_check
May 7, 2026
Merged

Add startup check for missing database indexes#48696
ahus1 merged 2 commits intokeycloak:mainfrom
pruivo:t_48695_db_index_check

Conversation

@pruivo
Copy link
Copy Markdown
Member

@pruivo pruivo commented May 4, 2026 •
edited
Loading

Closes #48695

  • Check dropTable changes - those remove indexes too.

@pruivo
Copy link
Copy Markdown
Member Author

pruivo commented May 4, 2026

ps. I haven't tested it out yet, just to run the testsuite.

ahus1
ahus1 previously requested changes May 4, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Hello Pedro, great to see this progressing!

See below for some suggested changes.

Comment thread ...me/src/main/java/org/keycloak/quarkus/runtime/storage/database/jpa/DatabaseIndexChecker.java Outdated
Comment thread ...me/src/main/java/org/keycloak/quarkus/runtime/storage/database/jpa/DatabaseIndexChecker.java Outdated
.map(Map.Entry::getValue)
.toList();
} catch (SQLException | LiquibaseException e) {
logger.debug("Unable to check for missing database indexes", e);
Copy link
Copy Markdown
Member

@ahus1 ahus1 May 4, 2026

Choose a reason for hiding this comment

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

Should be a warning, as we want to be sure that if no log message is there, that all indexes exist as expected. With a debug log we can't do that.

Comment thread ...me/src/main/java/org/keycloak/quarkus/runtime/storage/database/jpa/DatabaseIndexChecker.java Outdated
Set<String> existingIndexes = new HashSet<>();

for (String table : tables) {
try (ResultSet rs = metaData.getIndexInfo(null, dbSchema, table, false, true)) {
Copy link
Copy Markdown
Member

@ahus1 ahus1 May 4, 2026

Choose a reason for hiding this comment

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

It seems that some indexes are reported missing here, but the tables that they connect to are also missing. So with that, please ignore all indexes where the table is missing.

Copy link
Copy Markdown
Member

@ahus1 ahus1 May 4, 2026

Choose a reason for hiding this comment

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

Instead of where the table is missing, maybe "where the table has been deleted" might also be possible with the data from liquibase.

Copy link
Copy Markdown
Member Author

@pruivo pruivo May 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I've changed to intercept the dropTable changes, and the logs are clean now.

@pruivo pruivo force-pushed the t_48695_db_index_check branch 3 times, most recently from a80f469 to f2af3d9 Compare May 5, 2026 18:14
@pruivo
Add startup check for missing database indexes
228c3bc 
Closes keycloak#48695

Signed-off-by: Pedro Ruivo <1492066+pruivo@users.noreply.github.com>
@pruivo pruivo force-pushed the t_48695_db_index_check branch from f2af3d9 to 228c3bc Compare May 5, 2026 19:43
keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed May 5, 2026
View reviewed changes
Copy link
Copy Markdown

@keycloak-github-bot keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot
Copy link
Copy Markdown

keycloak-github-bot Bot commented May 5, 2026

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testReloginWithInvalidAuthSessionCookie

Keycloak CI - Adapter IT Strict Cookies

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in org.keycloak.testsuite.util.URLAssert URL expected to begin with: https://localhost:8543/auth/realms/demo/login-action ; actual URL: https://localhost:8643/sales-post-sig-email/ ==> expected: <true> but was: <false> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1160)
...

Report flaky test

@pruivo
Copy link
Copy Markdown
Member Author

pruivo commented May 6, 2026

On hold until #48736 and #48737 are merged.

@pruivo pruivo added the status/hold PR should not be merged. On hold for later. label May 6, 2026
@ahus1 ahus1 removed the status/hold PR should not be merged. On hold for later. label May 6, 2026
@ahus1
Evaluate all changelog preconditions as we're iterating over the chan…
b652e77 
…gelog with their cumulated state

Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
@ahus1 ahus1 force-pushed the t_48695_db_index_check branch from 09daa46 to b652e77 Compare May 6, 2026 18:40
@ahus1 ahus1 dismissed their stale review May 6, 2026 21:23

obsolete

@ahus1
Copy link
Copy Markdown
Member

ahus1 commented May 7, 2026

@pruivo - I tried to retrieve the indexes from H2, but there are just too many autogenerated ones which is very confusing.

I then debugged a startup with PostgreSQL and it expected 106 indexes from the parsed Liquibase files. And then I asked IntelliJ to create a full schema export to SQL and this also showed 106 indexes. So I think this is now good enough.

Please let me know your thoughts.

@pruivo
Copy link
Copy Markdown
Member Author

pruivo commented May 7, 2026

@pruivo - I tried to retrieve the indexes from H2, but there are just too many autogenerated ones which is very confusing.

I then debugged a startup with PostgreSQL and it expected 106 indexes from the parsed Liquibase files. And then I asked IntelliJ to create a full schema export to SQL and this also showed 106 indexes. So I think this is now good enough.

Please let me know your thoughts.

It looks good to me. I tried last night with MSSQL as well and manually confirmed the expectedIndexes and existingIndexes (excluding the constraints, they are shown as indexes) didn't miss anything.

This wouldn't have detected the mistake in the Liquibase changelog with the missing index. But I'm ok to merge if you are.

@pruivo pruivo marked this pull request as ready for review May 7, 2026 08:03
@pruivo pruivo requested review from a team as code owners May 7, 2026 08:03
@ahus1 ahus1 merged commit 53f0251 into keycloak:main May 7, 2026
90 checks passed
@pruivo pruivo deleted the t_48695_db_index_check branch May 7, 2026 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahus1 ahus1 ahus1 approved these changes

+1 more reviewer

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ahus1 ahus1

Labels

flaky-test team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add startup check for missing database indexes

2 participants

@pruivo @ahus1