From 208e0e9c23ddebefa181e3f8c9a4dad9c298d9a3 Mon Sep 17 00:00:00 2001 From: Toshihiro Suzuki Date: Tue, 20 May 2025 07:21:14 +0000 Subject: [PATCH 1/2] Empty commit [skip ci] From 77f34a434fa0c9e8a595081451b055c911bf5f7a Mon Sep 17 00:00:00 2001 From: Toshihiro Suzuki Date: Tue, 20 May 2025 16:20:56 +0900 Subject: [PATCH 2/2] Optimize JDBC storage by using Connection.setReadOnly() (#2651) --- .../com/scalar/db/storage/jdbc/JdbcAdmin.java | 58 +++++++++++-------- .../scalar/db/storage/jdbc/JdbcDatabase.java | 2 + .../com/scalar/db/storage/jdbc/JdbcUtils.java | 8 +++ .../db/storage/jdbc/RdbEngineSqlite.java | 6 ++ .../db/storage/jdbc/RdbEngineStrategy.java | 5 ++ .../db/storage/jdbc/JdbcAdminTestBase.java | 42 +++++++++++--- .../db/storage/jdbc/JdbcDatabaseTest.java | 4 ++ .../scalar/db/storage/jdbc/JdbcUtilsTest.java | 6 ++ 8 files changed, 99 insertions(+), 32 deletions(-) diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java index 66f3a1fde7..44c4da0660 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java @@ -394,6 +394,8 @@ public TableMetadata getTableMetadata(String namespace, String table) throws Exe boolean tableExists = false; try (Connection connection = dataSource.getConnection()) { + rdbEngine.setReadOnly(connection, true); + if (!namespaceExistsInternal(connection, metadataSchema)) { return null; } @@ -459,6 +461,8 @@ public TableMetadata getImportTableMetadata( } try (Connection connection = dataSource.getConnection()) { + rdbEngine.setReadOnly(connection, true); + String catalogName = rdbEngine.getCatalogName(namespace); String schemaName = rdbEngine.getSchemaName(namespace); @@ -545,19 +549,22 @@ public Set getNamespaceTableNames(String namespace) throws ExecutionExce + " WHERE " + enclose(METADATA_COL_FULL_TABLE_NAME) + " LIKE ?"; - try (Connection connection = dataSource.getConnection(); - PreparedStatement preparedStatement = - connection.prepareStatement(selectTablesOfNamespaceStatement)) { - String prefix = namespace + "."; - preparedStatement.setString(1, prefix + "%"); - try (ResultSet results = preparedStatement.executeQuery()) { - Set tableNames = new HashSet<>(); - while (results.next()) { - String tableName = - results.getString(METADATA_COL_FULL_TABLE_NAME).substring(prefix.length()); - tableNames.add(tableName); + try (Connection connection = dataSource.getConnection()) { + rdbEngine.setReadOnly(connection, true); + + try (PreparedStatement preparedStatement = + connection.prepareStatement(selectTablesOfNamespaceStatement)) { + String prefix = namespace + "."; + preparedStatement.setString(1, prefix + "%"); + try (ResultSet results = preparedStatement.executeQuery()) { + Set tableNames = new HashSet<>(); + while (results.next()) { + String tableName = + results.getString(METADATA_COL_FULL_TABLE_NAME).substring(prefix.length()); + tableNames.add(tableName); + } + return tableNames; } - return tableNames; } } catch (SQLException e) { // An exception will be thrown if the metadata table does not exist when executing the select @@ -576,6 +583,8 @@ public boolean namespaceExists(String namespace) throws ExecutionException { } try (Connection connection = dataSource.getConnection()) { + rdbEngine.setReadOnly(connection, true); + return namespaceExistsInternal(connection, namespace); } catch (SQLException e) { throw new ExecutionException("Checking if the namespace exists failed", e); @@ -804,18 +813,21 @@ public Set getNamespaceNames() throws ExecutionException { + enclose(METADATA_COL_FULL_TABLE_NAME) + " FROM " + encloseFullTableName(metadataSchema, METADATA_TABLE); - try (Connection connection = dataSource.getConnection(); - Statement stmt = connection.createStatement(); - ResultSet rs = stmt.executeQuery(selectAllTableNames)) { - Set namespaceOfExistingTables = new HashSet<>(); - while (rs.next()) { - String fullTableName = rs.getString(METADATA_COL_FULL_TABLE_NAME); - String namespaceName = fullTableName.substring(0, fullTableName.indexOf('.')); - namespaceOfExistingTables.add(namespaceName); - } - namespaceOfExistingTables.add(metadataSchema); + try (Connection connection = dataSource.getConnection()) { + rdbEngine.setReadOnly(connection, true); + + try (Statement stmt = connection.createStatement(); + ResultSet rs = stmt.executeQuery(selectAllTableNames)) { + Set namespaceOfExistingTables = new HashSet<>(); + while (rs.next()) { + String fullTableName = rs.getString(METADATA_COL_FULL_TABLE_NAME); + String namespaceName = fullTableName.substring(0, fullTableName.indexOf('.')); + namespaceOfExistingTables.add(namespaceName); + } + namespaceOfExistingTables.add(metadataSchema); - return namespaceOfExistingTables; + return namespaceOfExistingTables; + } } catch (SQLException e) { // An exception will be thrown if the namespace table does not exist when executing the select // query diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java index fd3da07557..7a17ea1a34 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java @@ -82,6 +82,7 @@ public Optional get(Get get) throws ExecutionException { Connection connection = null; try { connection = dataSource.getConnection(); + rdbEngine.setReadOnly(connection, true); return jdbcService.get(get, connection); } catch (SQLException e) { throw new ExecutionException( @@ -97,6 +98,7 @@ public Scanner scan(Scan scan) throws ExecutionException { Connection connection = null; try { connection = dataSource.getConnection(); + rdbEngine.setReadOnly(connection, true); return jdbcService.getScanner(scan, connection); } catch (SQLException e) { close(connection); diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcUtils.java b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcUtils.java index c4889f577b..81afc35bb4 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcUtils.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcUtils.java @@ -63,6 +63,8 @@ public static BasicDataSource initDataSource( } }); + dataSource.setDefaultReadOnly(false); + dataSource.setMinIdle(config.getConnectionPoolMinIdle()); dataSource.setMaxIdle(config.getConnectionPoolMaxIdle()); dataSource.setMaxTotal(config.getConnectionPoolMaxTotal()); @@ -89,6 +91,9 @@ public static BasicDataSource initDataSourceForTableMetadata( dataSource.setUrl(config.getJdbcUrl()); config.getUsername().ifPresent(dataSource::setUsername); config.getPassword().ifPresent(dataSource::setPassword); + + dataSource.setDefaultReadOnly(false); + dataSource.setMinIdle(config.getTableMetadataConnectionPoolMinIdle()); dataSource.setMaxIdle(config.getTableMetadataConnectionPoolMaxIdle()); dataSource.setMaxTotal(config.getTableMetadataConnectionPoolMaxTotal()); @@ -113,6 +118,9 @@ public static BasicDataSource initDataSourceForAdmin( dataSource.setUrl(config.getJdbcUrl()); config.getUsername().ifPresent(dataSource::setUsername); config.getPassword().ifPresent(dataSource::setPassword); + + dataSource.setDefaultReadOnly(false); + dataSource.setMinIdle(config.getAdminConnectionPoolMinIdle()); dataSource.setMaxIdle(config.getAdminConnectionPoolMaxIdle()); dataSource.setMaxTotal(config.getAdminConnectionPoolMaxTotal()); diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlite.java b/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlite.java index 466a7186bf..10495c96f9 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlite.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlite.java @@ -13,6 +13,7 @@ import com.scalar.db.storage.jdbc.query.UpsertQuery; import com.scalar.db.util.TimeRelatedColumnEncodingUtils; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import java.sql.Connection; import java.sql.Driver; import java.sql.JDBCType; import java.sql.ResultSet; @@ -336,4 +337,9 @@ public TimestampTZColumn parseTimestampTZColumn(ResultSet resultSet, String colu public RdbEngineTimeTypeStrategy getTimeTypeStrategy() { return timeTypeEngine; } + + @Override + public void setReadOnly(Connection connection, boolean readOnly) { + // Do nothing. SQLite does not support read-only mode. + } } diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java b/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java index c923c91035..e14ce77254 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java @@ -10,6 +10,7 @@ import com.scalar.db.io.TimestampTZColumn; import com.scalar.db.storage.jdbc.query.SelectQuery; import com.scalar.db.storage.jdbc.query.UpsertQuery; +import java.sql.Connection; import java.sql.Driver; import java.sql.JDBCType; import java.sql.ResultSet; @@ -226,4 +227,8 @@ default String getProjectionsSqlForSelectQuery(TableMetadata metadata, List actualTableNames = admin.getNamespaceTableNames(namespace); // Assert + if (rdbEngine == RdbEngine.SQLITE) { + verify(connection, never()).setReadOnly(anyBoolean()); + } else { + verify(connection).setReadOnly(true); + } verify(connection).prepareStatement(expectedSelectStatement); assertThat(actualTableNames).containsExactly(table1, table2); verify(preparedStatement).setString(1, namespace + ".%"); @@ -1771,6 +1785,11 @@ private void namespaceExists_forXWithExistingNamespace_ShouldReturnTrue( // Assert assertThat(admin.namespaceExists(namespace)).isTrue(); + if (rdbEngine == RdbEngine.SQLITE) { + verify(connection, never()).setReadOnly(anyBoolean()); + } else { + verify(connection).setReadOnly(true); + } verify(selectStatement).executeQuery(); verify(connection).prepareStatement(expectedSelectStatement); verify(selectStatement).setString(1, namespace + namespacePlaceholderSuffix); @@ -2772,16 +2791,15 @@ private void addNewColumnToTable_ForX_ShouldWorkProperly( } } - @Test - public void getImportTableMetadata_ForX_ShouldWorkProperly() + @ParameterizedTest + @EnumSource(RdbEngine.class) + public void getImportTableMetadata_ForX_ShouldWorkProperly(RdbEngine rdbEngine) throws SQLException, ExecutionException { - for (RdbEngine rdbEngine : RDB_ENGINES.keySet()) { - if (rdbEngine.equals(RdbEngine.SQLITE)) { - getImportTableMetadata_ForSQLite_ShouldThrowUnsupportedOperationException(rdbEngine); - } else { - getImportTableMetadata_ForOtherThanSQLite_ShouldWorkProperly( - rdbEngine, prepareSqlForTableCheck(rdbEngine, NAMESPACE, TABLE)); - } + if (rdbEngine.equals(RdbEngine.SQLITE)) { + getImportTableMetadata_ForSQLite_ShouldThrowUnsupportedOperationException(rdbEngine); + } else { + getImportTableMetadata_ForOtherThanSQLite_ShouldWorkProperly( + rdbEngine, prepareSqlForTableCheck(rdbEngine, NAMESPACE, TABLE)); } } @@ -2848,6 +2866,7 @@ public Boolean answer(InvocationOnMock invocation) { .execute(expectedCheckTableExistStatement); assertThat(actual.getPartitionKeyNames()).hasSameElementsAs(ImmutableSet.of("pk1", "pk2")); assertThat(actual.getColumnDataTypes()).containsExactlyEntriesOf(expectedColumns); + verify(connection).setReadOnly(true); verify(rdbEngineStrategy) .getDataTypeForScalarDb( any(JDBCType.class), @@ -3310,6 +3329,11 @@ private void getNamespaceNames_ForX_WithExistingTables_ShouldWorkProperly( Set actual = admin.getNamespaceNames(); // Assert + if (rdbEngine.equals(RdbEngine.SQLITE)) { + verify(connection, never()).setReadOnly(anyBoolean()); + } else { + verify(connection).setReadOnly(true); + } verify(connection).createStatement(); verify(getTableMetadataNamespacesStatementMock) .executeQuery(getTableMetadataNamespacesStatement); diff --git a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java index 8b88e36814..82b40061a6 100644 --- a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java +++ b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java @@ -71,6 +71,7 @@ public void whenGetOperationExecuted_shouldCallJdbcService() throws Exception { jdbcDatabase.get(get); // Assert + verify(connection).setReadOnly(true); verify(jdbcService).get(any(), any()); verify(connection).close(); } @@ -89,6 +90,7 @@ public void whenGetOperationExecuted_shouldCallJdbcService() throws Exception { jdbcDatabase.get(get); }) .isInstanceOf(ExecutionException.class); + verify(connection).setReadOnly(true); verify(connection).close(); } @@ -104,6 +106,7 @@ public void whenScanOperationExecutedAndScannerClosed_shouldCallJdbcService() th scanner.close(); // Assert + verify(connection).setReadOnly(true); verify(jdbcService).getScanner(any(), any()); verify(connection).close(); } @@ -122,6 +125,7 @@ public void whenScanOperationExecutedAndScannerClosed_shouldCallJdbcService() th jdbcDatabase.scan(scan); }) .isInstanceOf(ExecutionException.class); + verify(connection).setReadOnly(true); verify(connection).close(); } diff --git a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcUtilsTest.java b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcUtilsTest.java index 6b3b3edada..ce3e4f7ed7 100644 --- a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcUtilsTest.java +++ b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcUtilsTest.java @@ -66,6 +66,7 @@ public void initDataSource_NonTransactional_ShouldReturnProperDataSource() throw assertThat(dataSource.getAutoCommitOnReturn()).isEqualTo(true); assertThat(dataSource.getDefaultTransactionIsolation()) .isEqualTo(Connection.TRANSACTION_SERIALIZABLE); + assertThat(dataSource.getDefaultReadOnly()).isFalse(); assertThat(dataSource.getMinIdle()).isEqualTo(10); assertThat(dataSource.getMaxIdle()).isEqualTo(20); @@ -109,6 +110,7 @@ public void initDataSource_Transactional_ShouldReturnProperDataSource() throws S assertThat(dataSource.getAutoCommitOnReturn()).isEqualTo(false); assertThat(dataSource.getDefaultTransactionIsolation()) .isEqualTo(Connection.TRANSACTION_READ_COMMITTED); + assertThat(dataSource.getDefaultReadOnly()).isFalse(); assertThat(dataSource.getMinIdle()).isEqualTo(30); assertThat(dataSource.getMaxIdle()).isEqualTo(40); @@ -180,6 +182,8 @@ public void initDataSourceForTableMetadata_ShouldReturnProperDataSource() throws assertThat(tableMetadataDataSource.getUsername()).isEqualTo("user"); assertThat(tableMetadataDataSource.getPassword()).isEqualTo("oracle"); + assertThat(tableMetadataDataSource.getDefaultReadOnly()).isFalse(); + assertThat(tableMetadataDataSource.getMinIdle()).isEqualTo(100); assertThat(tableMetadataDataSource.getMaxIdle()).isEqualTo(200); assertThat(tableMetadataDataSource.getMaxTotal()).isEqualTo(300); @@ -212,6 +216,8 @@ public void initDataSourceForAdmin_ShouldReturnProperDataSource() throws SQLExce assertThat(adminDataSource.getUsername()).isEqualTo("user"); assertThat(adminDataSource.getPassword()).isEqualTo("sqlserver"); + assertThat(adminDataSource.getDefaultReadOnly()).isFalse(); + assertThat(adminDataSource.getMinIdle()).isEqualTo(100); assertThat(adminDataSource.getMaxIdle()).isEqualTo(200); assertThat(adminDataSource.getMaxTotal()).isEqualTo(300);