Skip to content

Commit

Permalink
feat: improve DB query performance
Browse files Browse the repository at this point in the history
- 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]
  • Loading branch information
peterhaochen47 committed Oct 24, 2024
1 parent 275857e commit 56a4b42
Show file tree
Hide file tree
Showing 10 changed files with 26 additions and 10 deletions.
Original file line number Diff line number Diff line change
@@ -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);
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TABLE credential
ADD COLUMN name_lowercase VARCHAR(1024) GENERATED ALWAYS AS (lower(name)) STORED;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CREATE INDEX credential_name_lowercase
ON credential(name_lowercase);
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TABLE credential
ADD COLUMN name_lowercase VARCHAR(1024) GENERATED ALWAYS AS (lower(name)) STORED;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CREATE INDEX CONCURRENTLY IF NOT EXISTS credential_name_lowercase
ON credential(name_lowercase);
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,23 @@ import java.util.UUID

interface CredentialRepository : JpaRepository<Credential?, UUID?> {
@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",
nativeQuery = true,
)
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",
Expand All @@ -33,7 +33,7 @@ interface CredentialRepository : JpaRepository<Credential?, UUID?> {
fun findAllCertificates(): List<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.name = ?1 limit 1 ",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
}
Expand Down

0 comments on commit 56a4b42

Please sign in to comment.