From 56a4b4209b9a324df6a8e49cd1b4f628d85fc21e Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Wed, 23 Oct 2024 11:11:21 -0700 Subject: [PATCH] feat: improve DB query performance - queries improved for MySQL and Postgres (including get-by-cred-name, find-by-name-like, delete-by-name) by performing case-insensitive search more efficiently details: - product requirements - The credential name itself is **case-sensitive** data (you store `/FooBar`, you get `/FooBar` back). But the cred search (credhub api's get-by-cred-name, get-by-cred-name-like, find-by-name endpoints) needs to be **case-insensitive** (you search `/fooBAR`, you still get `/FooBar` back). - CredHub has to work with MySQL, Postgres, and H2 (though H2 is only for testing). - problem statements (current state) - For MySQL, the queries currently uppercase or lowercase the search: `upper(credential.name) = upper(searched_term)` (sometimes it's uppercase, sometimes lowercase; it's not consistent), and without any relevant index, a full table scan is performed (I assume because it has to freshly compute the uppercased value of the name column for every single row). - For Postgres, a `lower(credential.name)` index is present, but some queries still do "uppercase" (those generated by Spring JPA lib from java code), resulting in full table scan. - solution: Add a new generated column that computes and stores the lowercased version of the credential names, and add index on it. - based on performance testing with local setup (240k cred entries in DB), this improves end-to-end performance by 3 times on average. Since this measurement includes fixed costs like local credhub cli processing time, local credhub server processing time, and local network latency, the DB query performance improvement must be much higher (more than 10 times based on MySQL team's analysis). - the generated column syntax in postgres & mysql does not quite work in H2 (despite H2 docs saying it works: https://h2database.com/html/features.html#generated_columns). Perhaps our H2 version is too old. But a similar syntax does work (tests passed). This is fine because we don't need to optimize H2 performance since it's just for testing. [internal tracking: https://vmw-jira.broadcom.net/browse/TPCF-26571] --- ..._add_lowercase_credential_name_column_and_index.sql | 5 +++++ .../V57__add_lowercase_credential_name_column.sql | 2 ++ .../mysql/V58__index_lowercase_credential_name.sql | 2 ++ .../V60__add_lowercase_credential_name_column.sql | 2 ++ .../postgres/V61__index_lowercase_credential_name.sql | 2 ++ .../credhub/services/CredentialDataService.java | 4 ++-- .../org/cloudfoundry/credhub/entity/Credential.kt | 3 +++ .../credhub/repositories/CredentialRepository.kt | 10 +++++----- .../services/DefaultCredentialVersionDataService.kt | 4 ++-- .../credhub/services/CredentialDataServiceTest.kt | 2 +- 10 files changed, 26 insertions(+), 10 deletions(-) create mode 100644 applications/credhub-api/src/main/resources/db/migration/h2/V59__add_lowercase_credential_name_column_and_index.sql create mode 100644 applications/credhub-api/src/main/resources/db/migration/mysql/V57__add_lowercase_credential_name_column.sql create mode 100644 applications/credhub-api/src/main/resources/db/migration/mysql/V58__index_lowercase_credential_name.sql create mode 100644 applications/credhub-api/src/main/resources/db/migration/postgres/V60__add_lowercase_credential_name_column.sql create mode 100644 applications/credhub-api/src/main/resources/db/migration/postgres/V61__index_lowercase_credential_name.sql diff --git a/applications/credhub-api/src/main/resources/db/migration/h2/V59__add_lowercase_credential_name_column_and_index.sql b/applications/credhub-api/src/main/resources/db/migration/h2/V59__add_lowercase_credential_name_column_and_index.sql new file mode 100644 index 000000000..40b1da299 --- /dev/null +++ b/applications/credhub-api/src/main/resources/db/migration/h2/V59__add_lowercase_credential_name_column_and_index.sql @@ -0,0 +1,5 @@ +ALTER TABLE credential + ADD COLUMN name_lowercase VARCHAR(1024) AS LOWER(name); + +CREATE INDEX credential_name_lowercase + ON credential(name_lowercase); \ No newline at end of file diff --git a/applications/credhub-api/src/main/resources/db/migration/mysql/V57__add_lowercase_credential_name_column.sql b/applications/credhub-api/src/main/resources/db/migration/mysql/V57__add_lowercase_credential_name_column.sql new file mode 100644 index 000000000..a48bb36fd --- /dev/null +++ b/applications/credhub-api/src/main/resources/db/migration/mysql/V57__add_lowercase_credential_name_column.sql @@ -0,0 +1,2 @@ +ALTER TABLE credential + ADD COLUMN name_lowercase VARCHAR(1024) GENERATED ALWAYS AS (lower(name)) STORED; \ No newline at end of file diff --git a/applications/credhub-api/src/main/resources/db/migration/mysql/V58__index_lowercase_credential_name.sql b/applications/credhub-api/src/main/resources/db/migration/mysql/V58__index_lowercase_credential_name.sql new file mode 100644 index 000000000..ed76031ae --- /dev/null +++ b/applications/credhub-api/src/main/resources/db/migration/mysql/V58__index_lowercase_credential_name.sql @@ -0,0 +1,2 @@ +CREATE INDEX credential_name_lowercase + ON credential(name_lowercase); \ No newline at end of file diff --git a/applications/credhub-api/src/main/resources/db/migration/postgres/V60__add_lowercase_credential_name_column.sql b/applications/credhub-api/src/main/resources/db/migration/postgres/V60__add_lowercase_credential_name_column.sql new file mode 100644 index 000000000..a48bb36fd --- /dev/null +++ b/applications/credhub-api/src/main/resources/db/migration/postgres/V60__add_lowercase_credential_name_column.sql @@ -0,0 +1,2 @@ +ALTER TABLE credential + ADD COLUMN name_lowercase VARCHAR(1024) GENERATED ALWAYS AS (lower(name)) STORED; \ No newline at end of file diff --git a/applications/credhub-api/src/main/resources/db/migration/postgres/V61__index_lowercase_credential_name.sql b/applications/credhub-api/src/main/resources/db/migration/postgres/V61__index_lowercase_credential_name.sql new file mode 100644 index 000000000..23292aeb3 --- /dev/null +++ b/applications/credhub-api/src/main/resources/db/migration/postgres/V61__index_lowercase_credential_name.sql @@ -0,0 +1,2 @@ +CREATE INDEX CONCURRENTLY IF NOT EXISTS credential_name_lowercase + ON credential(name_lowercase); \ No newline at end of file diff --git a/components/credentials/src/main/java/org/cloudfoundry/credhub/services/CredentialDataService.java b/components/credentials/src/main/java/org/cloudfoundry/credhub/services/CredentialDataService.java index 1fda2a180..7e5eb6034 100644 --- a/components/credentials/src/main/java/org/cloudfoundry/credhub/services/CredentialDataService.java +++ b/components/credentials/src/main/java/org/cloudfoundry/credhub/services/CredentialDataService.java @@ -23,7 +23,7 @@ public CredentialDataService(final CredentialRepository credentialRepository, fi } public Credential find(final String name) { - return credentialRepository.findOneByNameIgnoreCase(name); + return credentialRepository.findOneByNameLowercase(name.toLowerCase()); } public Credential findByUUID(final UUID uuid) { @@ -37,7 +37,7 @@ public Credential save(final Credential credential) { public boolean delete(final String credentialName) { final Credential cred = this.find(credentialName); auditRecord.setResource(cred); - return credentialRepository.deleteByNameIgnoreCase(credentialName) > 0; + return credentialRepository.deleteByNameLowercase(credentialName.toLowerCase()) > 0; } } diff --git a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/Credential.kt b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/Credential.kt index 8a58f38b4..ec1a36bce 100644 --- a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/Credential.kt +++ b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/Credential.kt @@ -41,6 +41,9 @@ class Credential : AuditableCredential { } } + @Column(name = "name_lowercase", nullable = false, insertable = false) + var nameLowercase: String? = null + @Column(unique = true, nullable = false) var checksum: String? = null diff --git a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/repositories/CredentialRepository.kt b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/repositories/CredentialRepository.kt index 7dec64627..b5e5ace91 100644 --- a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/repositories/CredentialRepository.kt +++ b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/repositories/CredentialRepository.kt @@ -8,12 +8,12 @@ import java.util.UUID interface CredentialRepository : JpaRepository { @Transactional - fun deleteByNameIgnoreCase(name: String?): Long + fun deleteByNameLowercase(name: String?): Long fun findOneByUuid(uuid: UUID?): Credential? @Query( - value = "select credential.uuid, credential.name, credential.checksum from certificate_credential " + + value = "select credential.uuid, credential.name, credential.name_lowercase, credential.checksum from certificate_credential " + "left join credential_version on certificate_credential.uuid = credential_version.uuid " + "join credential on credential.uuid = credential_version.credential_uuid " + "where credential.uuid = ?1", @@ -21,10 +21,10 @@ interface CredentialRepository : JpaRepository { ) fun findCertificateByUuid(uuid: UUID?): Credential? - fun findOneByNameIgnoreCase(name: String?): Credential? + fun findOneByNameLowercase(name: String?): Credential? @Query( - value = "select credential.uuid, credential.name, credential.checksum from certificate_credential " + + value = "select credential.uuid, credential.name, credential.name_lowercase, credential.checksum from certificate_credential " + "left join credential_version on certificate_credential.uuid = credential_version.uuid " + "join credential on credential.uuid = credential_version.credential_uuid " + "group by credential.uuid", @@ -33,7 +33,7 @@ interface CredentialRepository : JpaRepository { fun findAllCertificates(): List @Query( - value = "select credential.uuid, credential.name, credential.checksum from certificate_credential " + + value = "select credential.uuid, credential.name, credential.name_lowercase, credential.checksum from certificate_credential " + "left join credential_version on certificate_credential.uuid = credential_version.uuid " + "join credential on credential.uuid = credential_version.credential_uuid " + "where credential.name = ?1 limit 1 ", diff --git a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/services/DefaultCredentialVersionDataService.kt b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/services/DefaultCredentialVersionDataService.kt index c355f52a6..ce00858eb 100644 --- a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/services/DefaultCredentialVersionDataService.kt +++ b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/services/DefaultCredentialVersionDataService.kt @@ -264,11 +264,11 @@ constructor( (select version_created_at, credential_uuid from credential_version LEFT OUTER JOIN certificate_credential on credential_version.uuid = certificate_credential.uuid WHERE (transitional is false or transitional IS NULL) - and credential_uuid in (select uuid from credential where lower(name) like ?)) + and credential_uuid in (select uuid from credential where name_lowercase like ?)) as credential_version group by credential_uuid ) as credential_version inner join - (select * from credential where lower(name) like ? ) + (select * from credential where name_lowercase like ? ) as name on credential_version.credential_uuid = name.uuid order by version_created_at desc """.trimMargin() diff --git a/components/credentials/src/test/kotlin/org/cloudfoundry/credhub/services/CredentialDataServiceTest.kt b/components/credentials/src/test/kotlin/org/cloudfoundry/credhub/services/CredentialDataServiceTest.kt index a9eeb0938..0d90d9738 100644 --- a/components/credentials/src/test/kotlin/org/cloudfoundry/credhub/services/CredentialDataServiceTest.kt +++ b/components/credentials/src/test/kotlin/org/cloudfoundry/credhub/services/CredentialDataServiceTest.kt @@ -36,7 +36,7 @@ class CredentialDataServiceTest { credentialRepository.save(credential) MatcherAssert.assertThat(credentialRepository.count(), IsEqual.equalTo(1L)) MatcherAssert.assertThat( - credentialRepository.findOneByNameIgnoreCase(CREDENTIAL_NAME)!!.name, + credentialRepository.findOneByNameLowercase(CREDENTIAL_NAME.lowercase())!!.name, IsEqual.equalTo(CREDENTIAL_NAME), ) }