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 #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.
trinodb · Apr 12, 2021 · 9b33e49 · 9b33e49
1 parent a842287
commit 9b33e49
Show file tree

Hide file tree

Showing 6 changed files with 176 additions and 73 deletions.
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
@@ -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/...server/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerTransactionIsolationTest.java b/...server/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/...sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithSnapshotIsolation.java b/...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/...server/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolation.java b/...server/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/...trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolationAndReadCommittedSnapshot.java b/...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
@@ -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);
     }