From 9b33e4965f536ffefe0fafac13aa26f629189b97 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Sun, 11 Apr 2021 21:58:10 +0200 Subject: [PATCH] Fix SNAPSHOT ISOLATION detection in SQL Server Previously we were checking the flag set with `ALTER DATABASE ... SET READ_COMMITTED_SNAPSHOT ...` instead of the one set with `ALTER DATABASE ... SET ALLOW_SNAPSHOT_ISOLATION ...`. The two flags are related, but not the same. More explanation at https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/sql/snapshot-isolation-in-sql-server#snapshot-isolation-level-extensions This also simplifies the `TestingSqlServer` setup and improves coverage of snapshot isolation tests. - `TestingSqlServer` still enables snapshot isolation by default, to avoid regular tests being flaky on CI (as in https://github.com/trinodb/trino/issues/6389). The `READ_COMMITTED_SNAPSHOT` is enabled by default as well. - dedicated tests exercise the three remaining states of the `ALLOW_SNAPSHOT_ISOLATION` and `READ_COMMITTED_SNAPSHOT` options. These tests do not leverage `BaseConnectorSmokeTest` as this could re-introduce flakiness on CI. --- .../plugin/sqlserver/SqlServerClient.java | 2 +- ...BaseSqlServerTransactionIsolationTest.java | 89 +++++++++++++++++++ .../TestSqlServerWithSnapshotIsolation.java | 34 +++++++ ...TestSqlServerWithoutSnapshotIsolation.java | 71 +++------------ ...shotIsolationAndReadCommittedSnapshot.java | 34 +++++++ .../plugin/sqlserver/TestingSqlServer.java | 19 ++-- 6 files changed, 176 insertions(+), 73 deletions(-) create mode 100644 plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerTransactionIsolationTest.java create mode 100644 plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithSnapshotIsolation.java create mode 100644 plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolationAndReadCommittedSnapshot.java diff --git a/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java b/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java index 08843bf79fa22..4815b7be4b84f 100644 --- a/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java +++ b/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java @@ -558,7 +558,7 @@ private boolean hasSnapshotIsolationEnabled(Connection connection) try { return snapshotIsolationEnabled.get(SnapshotIsolationEnabledCacheKey.INSTANCE, () -> { Handle handle = Jdbi.open(connection); - return handle.createQuery("SELECT is_read_committed_snapshot_on FROM sys.databases WHERE name = :name") + return handle.createQuery("SELECT snapshot_isolation_state FROM sys.databases WHERE name = :name") .bind("name", connection.getCatalog()) .mapTo(Boolean.class) .findOne() diff --git a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerTransactionIsolationTest.java b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerTransactionIsolationTest.java new file mode 100644 index 0000000000000..09101c4266177 --- /dev/null +++ b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerTransactionIsolationTest.java @@ -0,0 +1,89 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.sqlserver; + +import io.trino.testing.AbstractTestQueryFramework; +import io.trino.testing.MaterializedResult; +import io.trino.testing.QueryRunner; +import org.testng.annotations.Test; + +import java.util.List; +import java.util.Map; + +import static io.trino.plugin.sqlserver.SqlServerQueryRunner.createSqlServerQueryRunner; +import static io.trino.spi.type.VarcharType.VARCHAR; +import static io.trino.tpch.TpchTable.NATION; +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; + +public abstract class BaseSqlServerTransactionIsolationTest + extends AbstractTestQueryFramework +{ + @Override + protected QueryRunner createQueryRunner() + throws Exception + { + TestingSqlServer sqlServer = closeAfterClass(new TestingSqlServer()); + sqlServer.start(); + configureDatabase(sqlServer); + return createSqlServerQueryRunner( + sqlServer, + Map.of(), + Map.of(), + List.of(NATION)); + } + + protected abstract void configureDatabase(TestingSqlServer sqlServer); + + @Test + public void testCreateReadTable() + { + assertUpdate("CREATE TABLE ctas_read AS SELECT * FROM tpch.tiny.nation", "SELECT count(*) FROM nation"); + assertQuery("SELECT AVG(LENGTH(name)) FROM ctas_read", "SELECT 7.08"); + assertQuery("SELECT SUM(LENGTH(name)) FROM ctas_read WHERE regionkey = 1", "SELECT 38"); + assertUpdate("DROP TABLE ctas_read"); + } + + @Test + public void testDescribeShowTable() + { + assertUpdate("CREATE TABLE ctas_describe AS SELECT regionkey, nationkey, comment FROM tpch.tiny.nation", "SELECT count(*) FROM nation"); + + MaterializedResult expectedColumns = MaterializedResult.resultBuilder(getQueryRunner().getDefaultSession(), VARCHAR, VARCHAR, VARCHAR, VARCHAR) + .row("regionkey", "bigint", "", "") + .row("nationkey", "bigint", "", "") + .row("comment", "varchar(152)", "", "") + .build(); + + MaterializedResult actualColumns = computeActual("DESCRIBE ctas_describe"); + assertThat(actualColumns).isEqualTo(expectedColumns); + + MaterializedResult expectedTables = MaterializedResult.resultBuilder(getQueryRunner().getDefaultSession(), VARCHAR) + .row("ctas_describe") + .build(); + + MaterializedResult actualTables = computeActual("SHOW TABLES LIKE 'ctas_describe'"); + assertThat(actualTables).isEqualTo(expectedTables); + + assertUpdate("DROP TABLE ctas_describe"); + } + + @Test + public void testCreateInsertReadTable() + { + assertUpdate("CREATE TABLE insert_table (col INTEGER)"); + assertUpdate("INSERT INTO insert_table (col) VALUES (1), (2), (3), (4)", 4); + assertQuery("SELECT AVG(col) FROM insert_table", "SELECT 2.5"); + assertUpdate("DROP TABLE insert_table"); + } +} diff --git a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithSnapshotIsolation.java b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithSnapshotIsolation.java new file mode 100644 index 0000000000000..a69f127ac8360 --- /dev/null +++ b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithSnapshotIsolation.java @@ -0,0 +1,34 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.sqlserver; + +import static java.lang.String.format; + +public class TestSqlServerWithSnapshotIsolation + extends BaseSqlServerTransactionIsolationTest +{ + @Override + protected void configureDatabase(TestingSqlServer sqlServer) + { + String databaseName = sqlServer.getDatabaseName(); + + // ALLOW_SNAPSHOT_ISOLATION controls whether SNAPSHOT ISOLATION is actually enabled + sqlServer.execute(format("ALTER DATABASE %s SET ALLOW_SNAPSHOT_ISOLATION ON", databaseName)); + + // READ_COMMITTED_SNAPSHOT that READ COMMITTED transaction isolation uses SNAPSHOT ISOLATION by default + // it has no effect when ALLOW_SNAPSHOT_ISOLATION is disabled + // https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/sql/snapshot-isolation-in-sql-server#snapshot-isolation-level-extensions + sqlServer.execute(format("ALTER DATABASE %s SET READ_COMMITTED_SNAPSHOT OFF", databaseName)); + } +} diff --git a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolation.java b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolation.java index c71961eeada57..8da557614a8f6 100644 --- a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolation.java +++ b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolation.java @@ -13,73 +13,22 @@ */ package io.trino.plugin.sqlserver; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import io.trino.testing.AbstractTestQueryFramework; -import io.trino.testing.MaterializedResult; -import io.trino.testing.QueryRunner; -import io.trino.tpch.TpchTable; -import org.testng.annotations.Test; - -import static io.trino.plugin.sqlserver.SqlServerQueryRunner.createSqlServerQueryRunner; -import static io.trino.spi.type.VarcharType.VARCHAR; -import static org.assertj.core.api.Assertions.assertThat; +import static java.lang.String.format; public class TestSqlServerWithoutSnapshotIsolation - extends AbstractTestQueryFramework + extends BaseSqlServerTransactionIsolationTest { @Override - protected QueryRunner createQueryRunner() - throws Exception - { - TestingSqlServer sqlServer = closeAfterClass(new TestingSqlServer(false)); - sqlServer.start(); - return createSqlServerQueryRunner( - sqlServer, - ImmutableMap.of(), - ImmutableMap.of(), - ImmutableList.of(TpchTable.NATION)); - } - - @Test - public void testCreateReadTable() - { - assertUpdate("CREATE TABLE ctas_read AS SELECT * FROM tpch.tiny.nation", "SELECT count(*) FROM nation"); - assertQuery("SELECT AVG(LENGTH(name)) FROM ctas_read", "SELECT 7.08"); - assertQuery("SELECT SUM(LENGTH(name)) FROM ctas_read WHERE regionkey = 1", "SELECT 38"); - assertUpdate("DROP TABLE ctas_read"); - } - - @Test - public void testDescribeShowTable() + protected void configureDatabase(TestingSqlServer sqlServer) { - assertUpdate("CREATE TABLE ctas_describe AS SELECT regionkey, nationkey, comment FROM tpch.tiny.nation", "SELECT count(*) FROM nation"); + String databaseName = sqlServer.getDatabaseName(); - MaterializedResult expectedColumns = MaterializedResult.resultBuilder(getQueryRunner().getDefaultSession(), VARCHAR, VARCHAR, VARCHAR, VARCHAR) - .row("regionkey", "bigint", "", "") - .row("nationkey", "bigint", "", "") - .row("comment", "varchar(152)", "", "") - .build(); + // ALLOW_SNAPSHOT_ISOLATION controls whether SNAPSHOT ISOLATION is actually enabled + sqlServer.execute(format("ALTER DATABASE %s SET ALLOW_SNAPSHOT_ISOLATION OFF", databaseName)); - MaterializedResult actualColumns = computeActual("DESCRIBE ctas_describe"); - assertThat(actualColumns).isEqualTo(expectedColumns); - - MaterializedResult expectedTables = MaterializedResult.resultBuilder(getQueryRunner().getDefaultSession(), VARCHAR) - .row("ctas_describe") - .build(); - - MaterializedResult actualTables = computeActual("SHOW TABLES LIKE 'ctas_describe'"); - assertThat(actualTables).isEqualTo(expectedTables); - - assertUpdate("DROP TABLE ctas_describe"); - } - - @Test - public void testCreateInsertReadTable() - { - assertUpdate("CREATE TABLE insert_table (col INTEGER)"); - assertUpdate("INSERT INTO insert_table (col) VALUES (1), (2), (3), (4)", 4); - assertQuery("SELECT AVG(col) FROM insert_table", "SELECT 2.5"); - assertUpdate("DROP TABLE insert_table"); + // READ_COMMITTED_SNAPSHOT that READ COMMITTED transaction isolation uses SNAPSHOT ISOLATION by default + // it has no effect when ALLOW_SNAPSHOT_ISOLATION is disabled + // https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/sql/snapshot-isolation-in-sql-server#snapshot-isolation-level-extensions + sqlServer.execute(format("ALTER DATABASE %s SET READ_COMMITTED_SNAPSHOT ON", databaseName)); } } diff --git a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolationAndReadCommittedSnapshot.java b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolationAndReadCommittedSnapshot.java new file mode 100644 index 0000000000000..72b96ddd4eccd --- /dev/null +++ b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolationAndReadCommittedSnapshot.java @@ -0,0 +1,34 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.sqlserver; + +import static java.lang.String.format; + +public class TestSqlServerWithoutSnapshotIsolationAndReadCommittedSnapshot + extends BaseSqlServerTransactionIsolationTest +{ + @Override + protected void configureDatabase(TestingSqlServer sqlServer) + { + String databaseName = sqlServer.getDatabaseName(); + + // ALLOW_SNAPSHOT_ISOLATION controls whether SNAPSHOT ISOLATION is actually enabled + sqlServer.execute(format("ALTER DATABASE %s SET ALLOW_SNAPSHOT_ISOLATION OFF", databaseName)); + + // READ_COMMITTED_SNAPSHOT that READ COMMITTED transaction isolation uses SNAPSHOT ISOLATION by default + // it has no effect when ALLOW_SNAPSHOT_ISOLATION is disabled + // https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/sql/snapshot-isolation-in-sql-server#snapshot-isolation-level-extensions + sqlServer.execute(format("ALTER DATABASE %s SET READ_COMMITTED_SNAPSHOT OFF", databaseName)); + } +} diff --git a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java index a4b6e2611c484..332835b2fd788 100644 --- a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java +++ b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java @@ -33,23 +33,21 @@ public final class TestingSqlServer private static final DockerImageName DOCKER_IMAGE_NAME = DockerImageName.parse("microsoft/mssql-server-linux:2017-CU13") .asCompatibleSubstituteFor("mcr.microsoft.com/mssql/server:2017-CU12"); private final MSSQLServerContainer container; - private final boolean snapshotIsolationEnabled; private final String databaseName; private Closeable cleanup = () -> {}; public TestingSqlServer() - { - this(true); - } - - public TestingSqlServer(boolean snapshotIsolationEnabled) { container = new MSSQLServerContainer<>(DOCKER_IMAGE_NAME); container.addEnv("ACCEPT_EULA", "yes"); - this.snapshotIsolationEnabled = snapshotIsolationEnabled; this.databaseName = "database_" + UUID.randomUUID().toString().replace("-", ""); } + public String getDatabaseName() + { + return databaseName; + } + public void execute(String sql) { try (Connection connection = container.createConnection(""); @@ -92,10 +90,9 @@ private void setUpDatabase() { execute("CREATE DATABASE " + databaseName); - if (snapshotIsolationEnabled) { - execute(format("ALTER DATABASE %s SET READ_COMMITTED_SNAPSHOT ON", databaseName)); - execute(format("ALTER DATABASE %s SET ALLOW_SNAPSHOT_ISOLATION ON", databaseName)); - } + // Enable snapshot isolation by default to reduce flakiness on CI + execute(format("ALTER DATABASE %s SET ALLOW_SNAPSHOT_ISOLATION ON", databaseName)); + execute(format("ALTER DATABASE %s SET READ_COMMITTED_SNAPSHOT ON", databaseName)); container.withUrlParam("database", this.databaseName); }