From 372d71d88db7320a4bf65fc2cfa2f99f86fe3744 Mon Sep 17 00:00:00 2001 From: Sasha Sheikin Date: Fri, 14 Oct 2022 10:18:15 +0200 Subject: [PATCH 1/2] Invalidate caches more coarsely Table handle contains column handles. When column handle is changed, data in cache for table handles is outdated. Test TableHandles cache invalidation on columns change. --- .../trino/plugin/jdbc/CachingJdbcClient.java | 14 ++++-- .../plugin/jdbc/TestCachingJdbcClient.java | 50 ++++++++++++++++++- .../plugin/jdbc/TestingH2JdbcClient.java | 8 +++ 3 files changed, 66 insertions(+), 6 deletions(-) diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java index 25b85a78e95c..2fa946c49c05 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java @@ -412,28 +412,28 @@ public void setTableComment(ConnectorSession session, JdbcTableHandle handle, Op public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column, Optional comment) { delegate.setColumnComment(session, handle, column, comment); - invalidateColumnsCache(handle.asPlainTable().getSchemaTableName()); + invalidateTableCaches(handle.asPlainTable().getSchemaTableName()); } @Override public void addColumn(ConnectorSession session, JdbcTableHandle handle, ColumnMetadata column) { delegate.addColumn(session, handle, column); - invalidateColumnsCache(handle.asPlainTable().getSchemaTableName()); + invalidateTableCaches(handle.asPlainTable().getSchemaTableName()); } @Override public void dropColumn(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column) { delegate.dropColumn(session, handle, column); - invalidateColumnsCache(handle.asPlainTable().getSchemaTableName()); + invalidateTableCaches(handle.asPlainTable().getSchemaTableName()); } @Override public void renameColumn(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle jdbcColumn, String newColumnName) { delegate.renameColumn(session, handle, jdbcColumn, newColumnName); - invalidateColumnsCache(handle.asPlainTable().getSchemaTableName()); + invalidateTableCaches(handle.asPlainTable().getSchemaTableName()); } @Override @@ -578,6 +578,12 @@ CacheStats getTableNamesCacheStats() return tableNamesCache.stats(); } + @VisibleForTesting + CacheStats getTableHandlesByNameCacheStats() + { + return tableHandlesByNameCache.stats(); + } + @VisibleForTesting CacheStats getTableHandlesByQueryCacheStats() { diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java index 8be4639e2dab..39213583364b 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java @@ -31,6 +31,7 @@ import io.trino.spi.statistics.Estimate; import io.trino.spi.statistics.TableStatistics; import io.trino.testing.TestingConnectorSession; +import org.jetbrains.annotations.NotNull; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -55,6 +56,7 @@ import static com.google.common.collect.ImmutableMap.toImmutableMap; import static io.airlift.concurrent.Threads.daemonThreadsNamed; import static io.trino.plugin.jdbc.TestCachingJdbcClient.CachingJdbcCache.STATISTICS_CACHE; +import static io.trino.plugin.jdbc.TestCachingJdbcClient.CachingJdbcCache.TABLE_HANDLES_BY_NAME_CACHE; import static io.trino.plugin.jdbc.TestCachingJdbcClient.CachingJdbcCache.TABLE_HANDLES_BY_QUERY_CACHE; import static io.trino.spi.session.PropertyMetadata.stringProperty; import static io.trino.spi.testing.InterfaceTestUtils.assertAllMethodsOverridden; @@ -273,6 +275,38 @@ public void testTableHandleOfQueryCached() dropTable(phantomTable); } + @Test + public void testTableHandleInvalidatedOnColumnsModifications() + { + JdbcTableHandle table = createTable(new SchemaTableName(schema, "a_table")); + JdbcColumnHandle existingColumn = addColumn(table, "a_column"); + + // warm-up cache + assertTableHandlesByNameCacheIsInvalidated(table); + JdbcColumnHandle newColumn = addColumn(cachingJdbcClient, table, "new_column"); + assertTableHandlesByNameCacheIsInvalidated(table); + cachingJdbcClient.setColumnComment(SESSION, table, newColumn, Optional.empty()); + assertTableHandlesByNameCacheIsInvalidated(table); + cachingJdbcClient.renameColumn(SESSION, table, newColumn, "new_column_name"); + assertTableHandlesByNameCacheIsInvalidated(table); + cachingJdbcClient.dropColumn(SESSION, table, existingColumn); + assertTableHandlesByNameCacheIsInvalidated(table); + + dropTable(table); + } + + private void assertTableHandlesByNameCacheIsInvalidated(JdbcTableHandle table) + { + SchemaTableName tableName = table.asPlainTable().getSchemaTableName(); + + assertCacheStats(cachingJdbcClient, TABLE_HANDLES_BY_NAME_CACHE).misses(1).loads(1).afterRunning(() -> { + assertThat(cachingJdbcClient.getTableHandle(SESSION, tableName).orElseThrow()).isEqualTo(table); + }); + assertCacheStats(cachingJdbcClient, TABLE_HANDLES_BY_NAME_CACHE).hits(1).afterRunning(() -> { + assertThat(cachingJdbcClient.getTableHandle(SESSION, tableName).orElseThrow()).isEqualTo(table); + }); + } + @Test public void testEmptyTableHandleIsCachedWhenCacheMissingIsTrue() { @@ -304,6 +338,11 @@ private JdbcTableHandle createTable(SchemaTableName phantomTable) return jdbcClient.getTableHandle(SESSION, phantomTable).orElseThrow(); } + private void dropTable(JdbcTableHandle tableHandle) + { + jdbcClient.dropTable(SESSION, tableHandle); + } + private void dropTable(SchemaTableName phantomTable) { JdbcTableHandle tableHandle = jdbcClient.getTableHandle(SESSION, phantomTable).orElseThrow(); @@ -819,10 +858,16 @@ private JdbcColumnHandle addColumn(JdbcTableHandle tableHandle) } private JdbcColumnHandle addColumn(JdbcTableHandle tableHandle, String columnName) + { + return addColumn(jdbcClient, tableHandle, columnName); + } + + @NotNull + private JdbcColumnHandle addColumn(JdbcClient client, JdbcTableHandle tableHandle, String columnName) { ColumnMetadata columnMetadata = new ColumnMetadata(columnName, INTEGER); - jdbcClient.addColumn(SESSION, tableHandle, columnMetadata); - return jdbcClient.getColumns(SESSION, tableHandle) + client.addColumn(SESSION, tableHandle, columnMetadata); + return client.getColumns(SESSION, tableHandle) .stream() .filter(jdbcColumnHandle -> jdbcColumnHandle.getColumnMetadata().equals(columnMetadata)) .findAny() @@ -995,6 +1040,7 @@ enum CachingJdbcCache { TABLE_NAMES_CACHE(CachingJdbcClient::getTableNamesCacheStats), TABLE_HANDLES_BY_QUERY_CACHE(CachingJdbcClient::getTableHandlesByQueryCacheStats), + TABLE_HANDLES_BY_NAME_CACHE(CachingJdbcClient::getTableHandlesByNameCacheStats), COLUMNS_CACHE(CachingJdbcClient::getColumnsCacheStats), STATISTICS_CACHE(CachingJdbcClient::getStatisticsCacheStats), /**/; diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java index 0a41962ea655..98d09cd1af34 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java @@ -41,6 +41,7 @@ import java.util.Map; import java.util.Optional; +import static com.google.common.base.Verify.verify; import static io.trino.plugin.jdbc.StandardColumnMappings.bigintColumnMapping; import static io.trino.plugin.jdbc.StandardColumnMappings.bigintWriteFunction; import static io.trino.plugin.jdbc.StandardColumnMappings.booleanColumnMapping; @@ -109,6 +110,13 @@ public Optional getTableComment(ResultSet resultSet) return Optional.empty(); } + @Override + public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column, Optional comment) + { + // do not throw when invoked, however do not allow to set non-empty comment until the connector supports setting column comments + verify(comment.isEmpty(), "This connector does not support setting column comments"); + } + @Override public boolean supportsAggregationPushdown(ConnectorSession session, JdbcTableHandle table, List aggregates, Map assignments, List> groupingSets) { From a54d48826b212fec968a4fa5956f0aa2895d9784 Mon Sep 17 00:00:00 2001 From: Sasha Sheikin Date: Mon, 24 Oct 2022 15:37:50 +0200 Subject: [PATCH 2/2] Fix verify in DefaultJdbcMetadata#applyProjection DeleteRowIdColumnHandle does not belong to the table handle however and is not designed to belong to it. This column handle required during analysis phase. The column is used for row-level delete. --- .../java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java index a3e10fc6161c..fdea0914fd7b 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java @@ -264,7 +264,12 @@ public Optional> applyProjecti return Optional.empty(); } - verify(tableColumnSet.containsAll(newColumnSet), "applyProjection called with columns %s and some are not available in existing query: %s", newColumnSet, tableColumnSet); + Set tableSyntheticColumnSet = ImmutableSet.builder() + .addAll(tableColumnSet) + .add((JdbcColumnHandle) getDeleteRowIdColumnHandle(session, table)) + .build(); + + verify(tableSyntheticColumnSet.containsAll(newColumnSet), "applyProjection called with columns %s and some are not available in existing query: %s", newColumnSet, tableSyntheticColumnSet); } return Optional.of(new ProjectionApplicationResult<>(