Optimize JDBC adaptor for MySQL #2801
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR aims to optimize the JDBC adapter for MySQL by refining connection property management and disabling performance-degrading configurations such as setting MySQL connections to read-only mode. Key changes include:
- Refactoring the getConnectionProperties API to accept a configuration parameter in multiple RdbEngine implementations.
- Disabling setting the read-only flag on MySQL connections to address performance degradation.
- Adjusting test cases to reflect changes in error handling and connection property configurations.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java | Changed RdbEngine strategy from MYSQL to POSTGRESQL and updated SQL error handling. |
| core/src/test/java/com/scalar/db/storage/jdbc/JdbcUtilsTest.java | Updated getConnectionProperties method signature usage in tests. |
| core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java | Changed RdbEngine strategy from MYSQL to POSTGRESQL and updated SQL error handling. |
| core/src/test/java/com/scalar/db/storage/jdbc/JdbcAdminTest.java | Adjusted read-only verification conditions to accommodate MySQL performance optimization. |
| core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java, RdbEngineSqlServer.java, RdbEngineMysql.java, RdbEngineMariaDB.java, RdbEngineDb2.java | Modified getConnectionProperties signatures to include JdbcConfig and introduced setConnectionToReadOnly for MySQL. |
| core/src/main/java/com/scalar/db/storage/jdbc/JdbcUtils.java | Updated loops to iterate using the new API signature. |
| core/src/main/java/com/scalar/db/storage/jdbc/JdbcConfig.java | Added a field and accessor for DatabaseConfig to support new API usage. |
Comments suppressed due to low confidence (2)
core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java:82
- The test case now uses the POSTGRESQL engine instead of MYSQL, which might not accurately test the MySQL optimizations addressed by this PR. Please verify if this change is intentional.
RdbEngine.createRdbEngineStrategy(RdbEngine.POSTGRESQL),
core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java:65
- Switching from MYSQL to POSTGRESQL in this test case could lead to inconsistencies when validating the MySQL-specific performance improvements. Please confirm if this change aligns with the intended testing scenarios for MySQL optimizations.
RdbEngine.createRdbEngineStrategy(RdbEngine.POSTGRESQL),
| @Override | ||
| public void setConnectionToReadOnly(Connection connection, boolean readOnly) throws SQLException { | ||
| // Observed performance degradation when using read-only connections in MySQL. So we do not | ||
| // set the read-only mode for MySQL connections. | ||
| } |
There was a problem hiding this comment.
We don't call Connection.setReadOnly(true) for MySQL.
There was a problem hiding this comment.
Just a question. We don't know the reason for the behavior?
There was a problem hiding this comment.
Right, we don't know the exact reason. We just observed performance degradation when we call Connection.setReadOnly(true) with MySQL.
| public Map<String, String> getConnectionProperties(JdbcConfig config) { | ||
| if (config.getDatabaseConfig().getScanFetchSize() == Integer.MIN_VALUE) { | ||
| // If the scan fetch size is set to Integer.MIN_VALUE, use the streaming mode. | ||
| return Collections.emptyMap(); | ||
| } | ||
|
|
||
| // Otherwise, use the cursor fetch mode. | ||
| return Collections.singletonMap("useCursorFetch", "true"); | ||
| } |
There was a problem hiding this comment.
If a user sets scalar.db.scan_fetch_size to Integer.MIN_VALUE, the streaming mode is used in MySQL. In that case, we should not set the connection property useCursorFetch=true.
| @Override | ||
| public void setConnectionToReadOnly(Connection connection, boolean readOnly) throws SQLException { | ||
| // Observed performance degradation when using read-only connections in MySQL. So we do not | ||
| // set the read-only mode for MySQL connections. | ||
| } |
There was a problem hiding this comment.
Just a question. We don't know the reason for the behavior?
| public Map<String, String> getConnectionProperties() { | ||
| public Map<String, String> getConnectionProperties(JdbcConfig config) { | ||
| if (config.getDatabaseConfig().getScanFetchSize() == Integer.MIN_VALUE) { | ||
| // If the scan fetch size is set to Integer.MIN_VALUE, use the streaming mode. |
There was a problem hiding this comment.
[question] Is this behavior only for MySQL?
There was a problem hiding this comment.
Don't need to add an integration test using scalar.db.scan_fetch_size = Integer.MIN_VALUE just in case ?
There was a problem hiding this comment.
Okay, let me do that.
There was a problem hiding this comment.
Added integration tests for it in bdb1309. Thanks!
brfrn169
force-pushed
the
optimize-jdbc-adopter-for-mysql
branch
from
0292941 to
bdb1309
Compare
4 participants
Description
When benchmarking the new release, I observed some performance degradation with the JDBC adapter for MySQL. After investigating the issue, I found the following:
Connection.setReadOnly(true)causes performance degradation.To address these issues, this PR introduces optimizations to the JDBC adapter for MySQL.
Related issues and/or PRs
N/A
Changes made
Added some inline comments. Please take a look for the details.
Checklist
Additional notes (optional)
N/A
Release notes
N/A