-
Notifications
You must be signed in to change notification settings - Fork 37
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.
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
Change default key column size and make it configurable for MySQL and Oracle #2245
Changes from 5 commits
7244a11
16e2aef
d2ff39b
b25bc82
b04a069
1072e98
5988dfc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,11 @@ public class JdbcConfig { | |
public static final String ADMIN_CONNECTION_POOL_MAX_TOTAL = | ||
PREFIX + "admin.connection_pool.max_total"; | ||
|
||
public static final String MYSQL_VARIABLE_KEY_COLUMN_SIZE = | ||
PREFIX + "mysql.variable_key_column_size"; | ||
public static final String ORACLE_VARIABLE_KEY_COLUMN_SIZE = | ||
PREFIX + "oracle.variable_key_column_size"; | ||
|
||
public static final int DEFAULT_CONNECTION_POOL_MIN_IDLE = 20; | ||
public static final int DEFAULT_CONNECTION_POOL_MAX_IDLE = 50; | ||
public static final int DEFAULT_CONNECTION_POOL_MAX_TOTAL = 200; | ||
|
@@ -61,6 +66,22 @@ public class JdbcConfig { | |
public static final int DEFAULT_ADMIN_CONNECTION_POOL_MAX_IDLE = 10; | ||
public static final int DEFAULT_ADMIN_CONNECTION_POOL_MAX_TOTAL = 25; | ||
|
||
// MySQL and Oracle have limitations regarding the total size of key columns. Thus, we should set | ||
// a small but enough key column size so that users can create multiple key columns without | ||
// exceeding the limit and changing the default. Since we found the old default size of 64 bytes | ||
// was small for some applications, we changed it based on the following specifications. See the | ||
// official documents for details. | ||
// 1) In MySQL, the maximum total size of key columns is 3072 bytes in the default, and thus, | ||
// depending on the charset, it can be up to 768 characters long. It can further be reduced if the | ||
// different settings are used. | ||
// 2) In Oracle, the maximum total size of key columns is approximately 75% of the database block | ||
// size minus some overhead. The default block size is 8KB, and it is typically 4kB or 8kB. Thus, | ||
// the maximum size can be similar to the MySQL. | ||
// See the official documents for details. | ||
// https://dev.mysql.com/doc/refman/8.0/en/innodb-limits.html | ||
// https://docs.oracle.com/en/database/oracle/oracle-database/23/refrn/logical-database-limits.html | ||
Comment on lines
+69
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
public static final int DEFAULT_VARIABLE_KEY_COLUMN_SIZE = 128; | ||
|
||
private final String jdbcUrl; | ||
@Nullable private final String username; | ||
@Nullable private final String password; | ||
|
@@ -82,6 +103,9 @@ public class JdbcConfig { | |
private final int adminConnectionPoolMaxIdle; | ||
private final int adminConnectionPoolMaxTotal; | ||
|
||
private final int mysqlVariableKeyColumnSize; | ||
private final int oracleVariableKeyColumnSize; | ||
|
||
public JdbcConfig(DatabaseConfig databaseConfig) { | ||
String storage = databaseConfig.getStorage(); | ||
String transactionManager = databaseConfig.getTransactionManager(); | ||
|
@@ -167,6 +191,22 @@ public JdbcConfig(DatabaseConfig databaseConfig) { | |
ADMIN_CONNECTION_POOL_MAX_TOTAL, | ||
DEFAULT_ADMIN_CONNECTION_POOL_MAX_TOTAL); | ||
|
||
mysqlVariableKeyColumnSize = | ||
getInt( | ||
databaseConfig.getProperties(), | ||
MYSQL_VARIABLE_KEY_COLUMN_SIZE, | ||
DEFAULT_VARIABLE_KEY_COLUMN_SIZE); | ||
|
||
oracleVariableKeyColumnSize = | ||
getInt( | ||
databaseConfig.getProperties(), | ||
ORACLE_VARIABLE_KEY_COLUMN_SIZE, | ||
DEFAULT_VARIABLE_KEY_COLUMN_SIZE); | ||
|
||
if (mysqlVariableKeyColumnSize < 64 || oracleVariableKeyColumnSize < 64) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's probably better to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @feeblefakie Definitely, thank you! I updated it with the reason based on Mitsu's comment in 1072e98. PTAL, just in case. |
||
throw new IllegalArgumentException(CoreError.INVALID_VARIABLE_KEY_COLUMN_SIZE.buildMessage()); | ||
} | ||
|
||
if (databaseConfig.getProperties().containsKey(TABLE_METADATA_SCHEMA)) { | ||
logger.warn( | ||
"The configuration property \"" | ||
|
@@ -250,4 +290,12 @@ public int getAdminConnectionPoolMaxIdle() { | |
public int getAdminConnectionPoolMaxTotal() { | ||
return adminConnectionPoolMaxTotal; | ||
} | ||
|
||
public int getMysqlVariableKeyColumnSize() { | ||
return mysqlVariableKeyColumnSize; | ||
} | ||
|
||
public int getOracleVariableKeyColumnSize() { | ||
return oracleVariableKeyColumnSize; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,6 @@ | |
import com.scalar.db.exception.storage.ExecutionException; | ||
import com.scalar.db.exception.storage.NoMutationException; | ||
import com.scalar.db.exception.storage.RetriableExecutionException; | ||
import com.scalar.db.storage.jdbc.query.QueryBuilder; | ||
import java.sql.Connection; | ||
import java.sql.SQLException; | ||
import java.util.List; | ||
|
@@ -60,8 +59,7 @@ public JdbcDatabase(DatabaseConfig databaseConfig) { | |
databaseConfig.getMetadataCacheExpirationTimeSecs()); | ||
|
||
OperationChecker operationChecker = new OperationChecker(databaseConfig, tableMetadataManager); | ||
QueryBuilder queryBuilder = new QueryBuilder(rdbEngine); | ||
jdbcService = new JdbcService(tableMetadataManager, operationChecker, queryBuilder); | ||
jdbcService = new JdbcService(tableMetadataManager, operationChecker, rdbEngine); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Push down |
||
} | ||
|
||
@VisibleForTesting | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
package com.scalar.db.storage.jdbc; | ||
|
||
import com.google.common.annotations.VisibleForTesting; | ||
import com.scalar.db.api.LikeExpression; | ||
import com.scalar.db.api.TableMetadata; | ||
import com.scalar.db.common.error.CoreError; | ||
|
@@ -21,6 +22,16 @@ | |
|
||
class RdbEngineMysql implements RdbEngineStrategy { | ||
private static final Logger logger = LoggerFactory.getLogger(RdbEngineMysql.class); | ||
private final String keyColumnSize; | ||
|
||
RdbEngineMysql(JdbcConfig config) { | ||
keyColumnSize = String.valueOf(config.getMysqlVariableKeyColumnSize()); | ||
} | ||
|
||
@VisibleForTesting | ||
RdbEngineMysql() { | ||
keyColumnSize = String.valueOf(JdbcConfig.DEFAULT_VARIABLE_KEY_COLUMN_SIZE); | ||
} | ||
|
||
@Override | ||
public String[] createSchemaSqls(String fullSchema) { | ||
|
@@ -201,9 +212,9 @@ public String getDataTypeForEngine(DataType scalarDbDataType) { | |
public String getDataTypeForKey(DataType dataType) { | ||
switch (dataType) { | ||
case TEXT: | ||
return "VARCHAR(64)"; | ||
return "VARCHAR(" + keyColumnSize + ")"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this column definition is also used for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @komamitsu Thank you! That's a really good point. We should check it maybe in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @komamitsu Revised in b04a069. PTAL! |
||
case BLOB: | ||
return "VARBINARY(64)"; | ||
return "VARBINARY(" + keyColumnSize + ")"; | ||
default: | ||
return null; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -204,6 +204,8 @@ public String getDataTypeForEngine(DataType scalarDbDataType) { | |
|
||
@Override | ||
public String getDataTypeForKey(DataType dataType) { | ||
// The number 10485760 is due to the maximum length of the character column. | ||
// https://www.postgresql.org/docs/15/datatype-character.html | ||
Comment on lines
+207
to
+208
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
if (dataType == DataType.TEXT) { | ||
return "VARCHAR(10485760)"; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I introduced two separate settings (only) for MySQL and Oracle for the following reasons.
jdbc.variable_key_column_size
does not affect it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reasoning for adding
variable
in the configuration name?In essence, it is a configuration, implying that the value is configurable (== variable). Following the same logic, we could have added
variable
in all the other configuration names.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Torch3333 Thank you for the question. It's a good point. It doesn't mean it's configurable, but the size of the variable (data-type) key column. I would like to distinguish it from other fixed-size data types like
int
anddouble
since this option only affects data types (e.g.,varchar
andvarbinary
) with variable length. Still weird?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok. I completely missed this interpretation. I agree with this reasoning. 👍
Thank you for the explanation.