From 55734e6cef7b479d964b9c724f1f692ab828ea71 Mon Sep 17 00:00:00 2001 From: Michael Buckley Date: Wed, 17 Jul 2024 13:27:47 -0400 Subject: [PATCH] Use lockless mode when adding index on Azure Sql server (#6100) * Use lockless mode when adding index on Azure Sql server Use try-catch for Online add-index on Sql Server. This avoids having to map out the entire matrix of Sql Server product names and ONLINE index support. Warnings in docs, and cleanups --- ...ort-online-migration-azure-sql-server.yaml | 4 + .../hapi/fhir/docs/server_jpa/upgrading.md | 6 ++ hapi-fhir-sql-migrate/pom.xml | 38 +++++++++ .../jpa/migrate/taskdef/AddIndexTask.java | 66 ++++++++++++--- .../jpa/migrate/taskdef/DropIndexTask.java | 7 +- .../jpa/migrate/taskdef/MetadataSource.java | 11 ++- .../taskdef/AddIndexTaskITTestSuite.java | 43 ++++++++++ .../jpa/migrate/taskdef/AddIndexTaskTest.java | 35 ++++---- .../taskdef/DropIndexTaskITTestSuite.java | 74 +++++++++++++++++ ...pIndexTest.java => DropIndexTaskTest.java} | 9 ++- .../migrate/taskdef/MetadataSourceTest.java | 10 ++- .../BaseCollectedMigrationTaskSuite.java | 66 +++++++++++++++ .../BaseMigrationTaskTestSuite.java | 81 +++++++++++++++++++ .../Postgres12CollectedMigrationTest.java | 25 ++++++ .../Postgres16CollectedMigrationTest.java | 25 ++++++ ...SqlServerAzureCollectedMigrationTests.java | 26 ++++++ ...rverEnterpriseCollectedMigrationTests.java | 22 +++++ ...lServerStandardCollectedMigrationTest.java | 28 +++++++ ...stContainerDatabaseMigrationExtension.java | 50 ++++++++++++ .../taskdef/containertests/package-info.java | 4 + 20 files changed, 588 insertions(+), 42 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6099-support-online-migration-azure-sql-server.yaml create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskITTestSuite.java create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTaskITTestSuite.java rename hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/{DropIndexTest.java => DropIndexTaskTest.java} (95%) create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/BaseCollectedMigrationTaskSuite.java create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/BaseMigrationTaskTestSuite.java create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/Postgres12CollectedMigrationTest.java create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/Postgres16CollectedMigrationTest.java create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerAzureCollectedMigrationTests.java create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerEnterpriseCollectedMigrationTests.java create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerStandardCollectedMigrationTest.java create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/TestContainerDatabaseMigrationExtension.java create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/package-info.java diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6099-support-online-migration-azure-sql-server.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6099-support-online-migration-azure-sql-server.yaml new file mode 100644 index 00000000000..eaab3c202ef --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6099-support-online-migration-azure-sql-server.yaml @@ -0,0 +1,4 @@ +--- +type: perf +issue: 6099 +title: "Database migrations that add or drop an index no longer lock tables when running on Azure Sql Server." diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa/upgrading.md b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa/upgrading.md index db88f84a419..f6ffb646df9 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa/upgrading.md +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa/upgrading.md @@ -31,6 +31,12 @@ Note that the Oracle JDBC drivers are not distributed in the Maven Central repos java -cp hapi-fhir-cli.jar ca.uhn.fhir.cli.App migrate-database -d ORACLE_12C -u "[url]" -n "[username]" -p "[password]" ``` +# Oracle and Sql Server Locking Note + +Some versions of Oracle and Sql Server (e.g. Oracle Standard or Sql Server Standard) do NOT support adding or removing an index without locking the underlying table. +If you run migrations while these systems are running, +they will have unavoidable long pauses in activity during these changes. + ## Migrating 3.4.0 to 3.5.0+ As of HAPI FHIR 3.5.0 a new mechanism for creating the JPA index tables (HFJ_SPIDX_xxx) has been implemented. This new mechanism uses hashes in place of large multi-column indexes. This improves both lookup times as well as required storage space. This change also paves the way for future ability to provide efficient multi-tenant searches (which is not yet implemented but is planned as an incremental improvement). diff --git a/hapi-fhir-sql-migrate/pom.xml b/hapi-fhir-sql-migrate/pom.xml index 55803f41b21..2010ab89a85 100644 --- a/hapi-fhir-sql-migrate/pom.xml +++ b/hapi-fhir-sql-migrate/pom.xml @@ -70,12 +70,50 @@ com.oracle.database.jdbc ojdbc11 + + com.microsoft.sqlserver + mssql-jdbc + test + ch.qos.logback logback-classic test + + ca.uhn.hapi.fhir + hapi-fhir-test-utilities + ${project.version} + test + + + org.testcontainers + junit-jupiter + test + + + org.testcontainers + postgresql + test + + + org.testcontainers + mssqlserver + test + + + org.testcontainers + oracle-xe + test + + + + + junit + junit + test + org.jetbrains annotations diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTask.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTask.java index 2251e19b2a8..5a94829e9a9 100644 --- a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTask.java +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTask.java @@ -33,6 +33,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Locale; +import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -43,7 +44,7 @@ public class AddIndexTask extends BaseTableTask { private String myIndexName; private List myColumns; private List myNullableColumns; - private Boolean myUnique; + private Boolean myUnique = false; private List myIncludeColumns = Collections.emptyList(); /** Should the operation avoid taking a lock on the table */ private boolean myOnline; @@ -79,7 +80,7 @@ public class AddIndexTask extends BaseTableTask { super.validate(); Validate.notBlank(myIndexName, "Index name not specified"); Validate.isTrue( - myColumns.size() > 0, + !myColumns.isEmpty(), "Columns not specified for AddIndexTask " + myIndexName + " on table " + getTableName()); Validate.notNull(myUnique, "Uniqueness not specified"); setDescription("Add " + myIndexName + " index to table " + getTableName()); @@ -151,7 +152,7 @@ public class AddIndexTask extends BaseTableTask { } // Should we do this non-transactionally? Avoids a write-lock, but introduces weird failure modes. String postgresOnlineClause = ""; - String msSqlOracleOnlineClause = ""; + String oracleOnlineClause = ""; if (myOnline) { switch (getDriverType()) { case POSTGRES_9_4: @@ -160,25 +161,66 @@ public class AddIndexTask extends BaseTableTask { // This runs without a lock, and can't be done transactionally. setTransactional(false); break; - case ORACLE_12C: - if (myMetadataSource.isOnlineIndexSupported(getConnectionProperties())) { - msSqlOracleOnlineClause = " ONLINE DEFERRED INVALIDATION"; - } - break; case MSSQL_2012: + // handled below in buildOnlineCreateWithTryCatchFallback() + break; + case ORACLE_12C: + // todo: delete this once we figure out how run Oracle try-catch to match MSSQL. if (myMetadataSource.isOnlineIndexSupported(getConnectionProperties())) { - msSqlOracleOnlineClause = " WITH (ONLINE = ON)"; + oracleOnlineClause = " ONLINE DEFERRED INVALIDATION"; } break; default: } } - String sql = "create " + unique + "index " + postgresOnlineClause + myIndexName + " on " + getTableName() + "(" - + columns + ")" + includeClause + mssqlWhereClause + msSqlOracleOnlineClause; + String bareCreateSql = "create " + unique + "index " + postgresOnlineClause + myIndexName + " on " + + getTableName() + "(" + columns + ")" + includeClause + mssqlWhereClause + oracleOnlineClause; + + String sql; + if (myOnline && DriverTypeEnum.MSSQL_2012 == getDriverType()) { + sql = buildOnlineCreateWithTryCatchFallback(bareCreateSql); + } else { + sql = bareCreateSql; + } return sql; } + /** + * Wrap a Sql Server create index in a try/catch to try it first ONLINE + * (meaning no table locks), and on failure, without ONLINE (locking the table). + * + * This try-catch syntax was manually tested via sql + * {@code + * BEGIN TRY + * EXEC('create index FOO on TABLE_A (col1) WITH (ONLINE = ON)'); + * select 'Online-OK'; + * END TRY + * BEGIN CATCH + * create index FOO on TABLE_A (col1); + * select 'Offline'; + * END CATCH; + * -- Then inspect the result set - Online-OK means it ran the ONLINE version. + * -- Note: we use EXEC() in the online path to lower the severity of the error + * -- so the CATCH can catch it. + * } + * + * @param bareCreateSql + * @return + */ + static @Nonnull String buildOnlineCreateWithTryCatchFallback(String bareCreateSql) { + // Some "Editions" of Sql Server do not support ONLINE. + // @format:off + return "BEGIN TRY -- try first online, without locking the table \n" + + " EXEC('" + bareCreateSql + " WITH (ONLINE = ON)');\n" + + "END TRY \n" + + "BEGIN CATCH -- for Editions of Sql Server that don't support ONLINE, run with table locks \n" + + bareCreateSql + + "; \n" + + "END CATCH;"; + // @format:on + } + @Nonnull private String buildMSSqlNotNullWhereClause() { String mssqlWhereClause = ""; @@ -207,7 +249,7 @@ public class AddIndexTask extends BaseTableTask { } private void setIncludeColumns(List theIncludeColumns) { - Validate.notNull(theIncludeColumns); + Objects.requireNonNull(theIncludeColumns); myIncludeColumns = theIncludeColumns; } diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTask.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTask.java index 80101f3ee3c..bb02eb0393e 100644 --- a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTask.java +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTask.java @@ -118,7 +118,12 @@ public class DropIndexTask extends BaseTableTask { sql.add("drop index " + myIndexName + (myOnline ? " ONLINE" : "")); break; case MSSQL_2012: - sql.add("drop index " + getTableName() + "." + myIndexName); + // use a try-catch to try online first, and fail over to lock path. + String sqlServerDrop = "drop index " + getTableName() + "." + myIndexName; + if (myOnline) { + sqlServerDrop = AddIndexTask.buildOnlineCreateWithTryCatchFallback(sqlServerDrop); + } + sql.add(sqlServerDrop); break; case COCKROACHDB_21_1: sql.add("drop index " + getTableName() + "@" + myIndexName); diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSource.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSource.java index 9a271968e7c..7a78f51edf0 100644 --- a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSource.java +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSource.java @@ -32,16 +32,23 @@ public class MetadataSource { */ public boolean isOnlineIndexSupported(DriverTypeEnum.ConnectionProperties theConnectionProperties) { + // todo: delete this once we figure out how run Oracle try-catch as well. switch (theConnectionProperties.getDriverType()) { case POSTGRES_9_4: case COCKROACHDB_21_1: return true; case MSSQL_2012: + // use a deny-list instead of allow list, so we have a better failure mode for new/unknown versions. + // Better to fail in dev than run with a table lock in production. String mssqlEdition = getEdition(theConnectionProperties); - return mssqlEdition.startsWith("Enterprise"); + return mssqlEdition == null // some weird version without an edition? + || + // these versions don't support ONLINE index creation + !mssqlEdition.startsWith("Standard Edition"); case ORACLE_12C: String oracleEdition = getEdition(theConnectionProperties); - return oracleEdition.contains("Enterprise"); + return oracleEdition == null // weird unknown version - try, and maybe fail. + || oracleEdition.contains("Enterprise"); default: return false; } diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskITTestSuite.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskITTestSuite.java new file mode 100644 index 00000000000..c06b397a22a --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskITTestSuite.java @@ -0,0 +1,43 @@ +package ca.uhn.fhir.jpa.migrate.taskdef; + +import ca.uhn.fhir.jpa.migrate.JdbcUtils; +import ca.uhn.fhir.jpa.migrate.taskdef.containertests.BaseMigrationTaskTestSuite; +import ca.uhn.fhir.jpa.migrate.tasks.api.Builder; +import org.assertj.core.api.Assertions; +import org.awaitility.Awaitility; +import org.junit.jupiter.api.Test; + +import java.sql.SQLException; +import java.util.concurrent.TimeUnit; + +/** + * Integration tests for AddIndexTask. + */ +public interface AddIndexTaskITTestSuite extends BaseMigrationTaskTestSuite { + + @Test + default void testAddIndexOnline_createsIndex() throws SQLException { + // given + Builder builder = getSupport().getBuilder(); + String tableName = "TABLE_ADD" + System.currentTimeMillis(); + Builder.BuilderAddTableByColumns tableBuilder = builder.addTableByColumns("1", tableName, "id"); + tableBuilder.addColumn("id").nonNullable().type(ColumnTypeEnum.LONG); + tableBuilder.addColumn("col1").nullable().type(ColumnTypeEnum.STRING, 100); + getSupport().executeAndClearPendingTasks(); + + // when + builder.onTable(tableName) + .addIndex("2", "FOO") + .unique(false) + .online(true) + .withColumns("col1"); + getSupport().executeAndClearPendingTasks(); + + // then + + // we wait since the ONLINE path is async. + Awaitility.await("index FOO exists").atMost(10, TimeUnit.SECONDS).untilAsserted( + () -> Assertions.assertThat(JdbcUtils.getIndexNames(getSupport().getConnectionProperties(), tableName)).contains("FOO")); + } + +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskTest.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskTest.java index d77535ff1b8..87684f6d30d 100644 --- a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskTest.java +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskTest.java @@ -20,6 +20,7 @@ import java.util.function.Supplier; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +@SuppressWarnings("SqlDialectInspection") @MockitoSettings(strictness = Strictness.LENIENT) public class AddIndexTaskTest extends BaseTest { @@ -178,7 +179,7 @@ public class AddIndexTaskTest extends BaseTest { public void platformSyntaxWhenOn(DriverTypeEnum theDriver) { myTask.setDriverType(theDriver); myTask.setOnline(true); - DriverTypeEnum.ConnectionProperties props; + Mockito.when(mockMetadataSource.isOnlineIndexSupported(Mockito.any())).thenReturn(true); mySql = myTask.generateSql(); switch (theDriver) { @@ -190,7 +191,12 @@ public class AddIndexTaskTest extends BaseTest { assertEquals("create index IDX_ANINDEX on SOMETABLE(PID, TEXTCOL) ONLINE DEFERRED INVALIDATION", mySql); break; case MSSQL_2012: - assertEquals("create index IDX_ANINDEX on SOMETABLE(PID, TEXTCOL) WITH (ONLINE = ON)", mySql); + assertEquals("BEGIN TRY -- try first online, without locking the table \n" + + " EXEC('create index IDX_ANINDEX on SOMETABLE(PID, TEXTCOL) WITH (ONLINE = ON)');\n" + + "END TRY \n" + + "BEGIN CATCH -- for Editions of Sql Server that don't support ONLINE, run with table locks \n" + + "create index IDX_ANINDEX on SOMETABLE(PID, TEXTCOL); \n" + + "END CATCH;", mySql); break; default: // unsupported is ok. But it means we lock the table for a bit. @@ -199,32 +205,19 @@ public class AddIndexTaskTest extends BaseTest { } } + /** + * We sniff the edition of Oracle to detect support for ONLINE migrations. + */ @ParameterizedTest(name = "{index}: {0}") @ValueSource(booleans = { true, false } ) - public void offForUnsupportedVersionsOfSqlServer(boolean theSupportedFlag) { - myTask.setDriverType(DriverTypeEnum.MSSQL_2012); - myTask.setOnline(true); - myTask.setMetadataSource(mockMetadataSource); - Mockito.when(mockMetadataSource.isOnlineIndexSupported(Mockito.any())).thenReturn(theSupportedFlag); - - mySql = myTask.generateSql(); - if (theSupportedFlag) { - assertEquals("create index IDX_ANINDEX on SOMETABLE(PID, TEXTCOL) WITH (ONLINE = ON)", mySql); - } else { - assertEquals("create index IDX_ANINDEX on SOMETABLE(PID, TEXTCOL)", mySql); - } - } - - @ParameterizedTest(name = "{index}: {0}") - @ValueSource(booleans = { true, false } ) - public void offForUnsupportedVersionsOfOracleServer(boolean theSupportedFlag) { + public void offForUnsupportedVersionsOfOracleServer(boolean theOnlineIndexingSupportedFlag) { myTask.setDriverType(DriverTypeEnum.ORACLE_12C); myTask.setOnline(true); myTask.setMetadataSource(mockMetadataSource); - Mockito.when(mockMetadataSource.isOnlineIndexSupported(Mockito.any())).thenReturn(theSupportedFlag); + Mockito.when(mockMetadataSource.isOnlineIndexSupported(Mockito.any())).thenReturn(theOnlineIndexingSupportedFlag); mySql = myTask.generateSql(); - if (theSupportedFlag) { + if (theOnlineIndexingSupportedFlag) { assertEquals("create index IDX_ANINDEX on SOMETABLE(PID, TEXTCOL) ONLINE DEFERRED INVALIDATION", mySql); } else { assertEquals("create index IDX_ANINDEX on SOMETABLE(PID, TEXTCOL)", mySql); diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTaskITTestSuite.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTaskITTestSuite.java new file mode 100644 index 00000000000..10e1f19f8b4 --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTaskITTestSuite.java @@ -0,0 +1,74 @@ +package ca.uhn.fhir.jpa.migrate.taskdef; + +import ca.uhn.fhir.jpa.migrate.JdbcUtils; +import ca.uhn.fhir.jpa.migrate.taskdef.containertests.BaseMigrationTaskTestSuite; +import ca.uhn.fhir.jpa.migrate.tasks.api.Builder; +import org.assertj.core.api.Assertions; +import org.awaitility.Awaitility; +import org.junit.jupiter.api.Test; + +import java.sql.SQLException; +import java.util.concurrent.TimeUnit; + +/** + * Integration tests for AddIndexTask. + */ +public interface DropIndexTaskITTestSuite extends BaseMigrationTaskTestSuite { + + + @Test + default void testDropIndex_dropsIndex() throws SQLException { + // given + Builder builder = getSupport().getBuilder(); + String tableName = "INDEX_DROP" + System.currentTimeMillis(); + Builder.BuilderAddTableByColumns tableBuilder = builder.addTableByColumns("1", tableName, "id"); + tableBuilder.addColumn("id").nonNullable().type(ColumnTypeEnum.LONG); + tableBuilder.addColumn("col1").nullable().type(ColumnTypeEnum.STRING, 100); + builder.onTable(tableName) + .addIndex("2", "FOO") + .unique(false) + .online(false) + .withColumns("col1"); + getSupport().executeAndClearPendingTasks(); + Assertions.assertThat(JdbcUtils.getIndexNames(getSupport().getConnectionProperties(), tableName)).contains("FOO"); + + // when + builder.onTable(tableName) + .dropIndex("2", "FOO"); + getSupport().executeAndClearPendingTasks(); + + // then + Assertions.assertThat(JdbcUtils.getIndexNames(getSupport().getConnectionProperties(), tableName)) + .as("index FOO does not exist") + .doesNotContain("FOO"); + } + + @Test + default void testDropIndexOnline_dropsIndex() throws SQLException { + // given + Builder builder = getSupport().getBuilder(); + String tableName = "INDEX_DROP" + System.currentTimeMillis(); + Builder.BuilderAddTableByColumns tableBuilder = builder.addTableByColumns("1", tableName, "id"); + tableBuilder.addColumn("id").nonNullable().type(ColumnTypeEnum.LONG); + tableBuilder.addColumn("col1").nullable().type(ColumnTypeEnum.STRING, 100); + builder.onTable(tableName) + .addIndex("2", "FOO") + .unique(false) + .online(false) + .withColumns("col1"); + getSupport().executeAndClearPendingTasks(); + Assertions.assertThat(JdbcUtils.getIndexNames(getSupport().getConnectionProperties(), tableName)).contains("FOO"); + + // when + builder.onTable(tableName) + .dropIndexOnline("2", "FOO"); + getSupport().executeAndClearPendingTasks(); + + // then + + // we wait since the ONLINE path is async. + Awaitility.await("index FOO does not exist").atMost(10, TimeUnit.SECONDS).untilAsserted( + () -> Assertions.assertThat(JdbcUtils.getIndexNames(getSupport().getConnectionProperties(), tableName)).doesNotContain("FOO")); + } + +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTest.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTaskTest.java similarity index 95% rename from hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTest.java rename to hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTaskTest.java index 7c85074754d..4e7f6d555cc 100644 --- a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTest.java +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTaskTest.java @@ -16,7 +16,7 @@ import static java.util.Arrays.asList; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; -public class DropIndexTest extends BaseTest { +public class DropIndexTaskTest extends BaseTest { @ParameterizedTest(name = "{index}: {0}") @@ -248,7 +248,12 @@ public class DropIndexTest extends BaseTest { assertEquals(asList("drop index IDX_ANINDEX ONLINE"), mySql); break; case MSSQL_2012: - assertEquals(asList("drop index SOMETABLE.IDX_ANINDEX"), mySql); + assertEquals(asList("BEGIN TRY -- try first online, without locking the table \n" + + " EXEC('drop index SOMETABLE.IDX_ANINDEX WITH (ONLINE = ON)');\n" + + "END TRY \n" + + "BEGIN CATCH -- for Editions of Sql Server that don't support ONLINE, run with table locks \n" + + "drop index SOMETABLE.IDX_ANINDEX; \n" + + "END CATCH;"), mySql); break; case POSTGRES_9_4: assertEquals(asList("drop index CONCURRENTLY IDX_ANINDEX"), mySql); diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSourceTest.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSourceTest.java index 1f92c18869f..22a5dd36789 100644 --- a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSourceTest.java +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSourceTest.java @@ -30,12 +30,14 @@ class MetadataSourceTest { "ORACLE_12C,Oracle Database 19c Enterprise Edition Release 19.0.0.0.0 - Production,true", "ORACLE_12C,Oracle Database 19c Express Edition Release 11.2.0.2.0 - 64bit Production,false", "COCKROACHDB_21_1,,true", - // sql server only supports it in Enterprise + // sql server only supports it in Enterprise and Developer // https://docs.microsoft.com/en-us/sql/sql-server/editions-and-components-of-sql-server-2019?view=sql-server-ver16#RDBMSHA - "MSSQL_2012,Developer Edition (64-bit),false", - "MSSQL_2012,Developer Edition (64-bit),false", + "MSSQL_2012,Developer Edition (64-bit),true", + "MSSQL_2012,Developer Edition (64-bit),true", "MSSQL_2012,Standard Edition (64-bit),false", - "MSSQL_2012,Enterprise Edition (64-bit),true" + "MSSQL_2012,Enterprise Edition (64-bit),true", + "MSSQL_2012,Azure SQL Edge Developer (64-bit),true", + "MSSQL_2012,Azure SQL Edge Premium (64-bit),true" }) void isOnlineIndexSupported(DriverTypeEnum theType, String theEdition, boolean theSupportedFlag) { // stub out our Sql Server edition lookup diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/BaseCollectedMigrationTaskSuite.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/BaseCollectedMigrationTaskSuite.java new file mode 100644 index 00000000000..957637ca8a4 --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/BaseCollectedMigrationTaskSuite.java @@ -0,0 +1,66 @@ +package ca.uhn.fhir.jpa.migrate.taskdef.containertests; + +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; +import ca.uhn.fhir.jpa.migrate.taskdef.AddIndexTaskITTestSuite; +import ca.uhn.fhir.jpa.migrate.taskdef.DropIndexTaskITTestSuite; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import jakarta.annotation.Nonnull; + +import static ca.uhn.fhir.jpa.migrate.taskdef.containertests.BaseMigrationTaskTestSuite.Support; + +/** + * Collects all our task suites in a single class so we can run them on each engine. + */ +public abstract class BaseCollectedMigrationTaskSuite { + + /** + * Per-test supplier for db-access, migration task list, etc. + */ + BaseMigrationTaskTestSuite.Support mySupport; + + @BeforeEach + void setUp() { + DriverTypeEnum.ConnectionProperties connectionProperties = getConnectionProperties(); + mySupport = Support.supportFrom(connectionProperties); + } + + /** + * Handle on concrete class container connection info. + */ + @Nonnull + protected abstract DriverTypeEnum.ConnectionProperties getConnectionProperties(); + + + + final public BaseMigrationTaskTestSuite.Support getSupport() { + return mySupport; + } + + + @Nested + class AddIndexTaskTests implements AddIndexTaskITTestSuite { + @Override + public Support getSupport() { + return BaseCollectedMigrationTaskSuite.this.getSupport(); + } + } + + @Nested + class DropIndexTaskTests implements DropIndexTaskITTestSuite { + @Override + public Support getSupport() { + return BaseCollectedMigrationTaskSuite.this.getSupport(); + } + } + + @Test + void testNothing() { + // an empty test to quiet sonar + Assertions.assertTrue(true); + } + +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/BaseMigrationTaskTestSuite.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/BaseMigrationTaskTestSuite.java new file mode 100644 index 00000000000..59445072221 --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/BaseMigrationTaskTestSuite.java @@ -0,0 +1,81 @@ +package ca.uhn.fhir.jpa.migrate.taskdef.containertests; + +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; +import ca.uhn.fhir.jpa.migrate.taskdef.BaseTask; +import ca.uhn.fhir.jpa.migrate.tasks.api.BaseMigrationTasks; +import ca.uhn.fhir.jpa.migrate.tasks.api.Builder; + +import java.sql.SQLException; +import java.util.LinkedList; + +/** + * Mixin for a migration task test suite + */ +public interface BaseMigrationTaskTestSuite { + Support getSupport(); + + class Support { + final TaskExecutor myTaskExecutor; + final Builder myBuilder; + final DriverTypeEnum.ConnectionProperties myConnectionProperties; + + public static Support supportFrom(DriverTypeEnum.ConnectionProperties theConnectionProperties) { + return new Support(theConnectionProperties); + } + + Support(DriverTypeEnum.ConnectionProperties theConnectionProperties) { + myConnectionProperties = theConnectionProperties; + myTaskExecutor = new TaskExecutor(theConnectionProperties); + myBuilder = new Builder("1.0", myTaskExecutor); + } + + public Builder getBuilder() { + return myBuilder; + } + + public void executeAndClearPendingTasks() throws SQLException { + myTaskExecutor.flushPendingTasks(); + } + + public DriverTypeEnum.ConnectionProperties getConnectionProperties() { + return myConnectionProperties; + } + } + + + /** + * Collect and execute the tasks from the Builder + */ + class TaskExecutor implements BaseMigrationTasks.IAcceptsTasks { + final DriverTypeEnum.ConnectionProperties myConnectionProperties; + final LinkedList myTasks = new LinkedList<>(); + + TaskExecutor(DriverTypeEnum.ConnectionProperties theConnectionProperties) { + myConnectionProperties = theConnectionProperties; + } + + /** + * Receive a task from the Builder + */ + public void addTask(BaseTask theTask) { + myTasks.add(theTask); + } + + /** + * Remove and execute each task in the list. + */ + public void flushPendingTasks() throws SQLException { + while (!myTasks.isEmpty()) { + executeTask(myTasks.removeFirst()); + } + } + + void executeTask(BaseTask theTask) throws SQLException { + theTask.setDriverType(myConnectionProperties.getDriverType()); + theTask.setConnectionProperties(myConnectionProperties); + theTask.execute(); + } + + } + +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/Postgres12CollectedMigrationTest.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/Postgres12CollectedMigrationTest.java new file mode 100644 index 00000000000..ae2bf836677 --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/Postgres12CollectedMigrationTest.java @@ -0,0 +1,25 @@ +package ca.uhn.fhir.jpa.migrate.taskdef.containertests; + +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.testcontainers.containers.PostgreSQLContainer; +import org.testcontainers.junit.jupiter.Testcontainers; + +import jakarta.annotation.Nonnull; + +@Testcontainers(disabledWithoutDocker=true) +public class Postgres12CollectedMigrationTest extends BaseCollectedMigrationTaskSuite { + + @RegisterExtension + static TestContainerDatabaseMigrationExtension ourContainerExtension = + new TestContainerDatabaseMigrationExtension( + DriverTypeEnum.POSTGRES_9_4, + new PostgreSQLContainer<>("postgres:12.2")); + + @Override + @Nonnull + protected DriverTypeEnum.ConnectionProperties getConnectionProperties() { + return ourContainerExtension.getConnectionProperties(); + } + +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/Postgres16CollectedMigrationTest.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/Postgres16CollectedMigrationTest.java new file mode 100644 index 00000000000..d1622cb0d9e --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/Postgres16CollectedMigrationTest.java @@ -0,0 +1,25 @@ +package ca.uhn.fhir.jpa.migrate.taskdef.containertests; + +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.testcontainers.containers.PostgreSQLContainer; +import org.testcontainers.junit.jupiter.Testcontainers; + +import jakarta.annotation.Nonnull; + +@Testcontainers(disabledWithoutDocker=true) +public class Postgres16CollectedMigrationTest extends BaseCollectedMigrationTaskSuite { + + @RegisterExtension + static TestContainerDatabaseMigrationExtension ourContainerExtension = + new TestContainerDatabaseMigrationExtension( + DriverTypeEnum.POSTGRES_9_4, + new PostgreSQLContainer<>("postgres:16.3")); + + @Override + @Nonnull + protected DriverTypeEnum.ConnectionProperties getConnectionProperties() { + return ourContainerExtension.getConnectionProperties(); + } + +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerAzureCollectedMigrationTests.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerAzureCollectedMigrationTests.java new file mode 100644 index 00000000000..e9224b6d56d --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerAzureCollectedMigrationTests.java @@ -0,0 +1,26 @@ +package ca.uhn.fhir.jpa.migrate.taskdef.containertests; + +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.testcontainers.containers.MSSQLServerContainer; +import org.testcontainers.utility.DockerImageName; + +import jakarta.annotation.Nonnull; + +public class SqlServerAzureCollectedMigrationTests extends BaseCollectedMigrationTaskSuite { + @RegisterExtension + static TestContainerDatabaseMigrationExtension ourContainerExtension = + new TestContainerDatabaseMigrationExtension( + DriverTypeEnum.MSSQL_2012, + new MSSQLServerContainer<>( + DockerImageName.parse("mcr.microsoft.com/azure-sql-edge:latest") + .asCompatibleSubstituteFor("mcr.microsoft.com/mssql/server")) + .withEnv("ACCEPT_EULA", "Y") + .withEnv("MSSQL_PID", "Premium")); // Product id: Azure Premium vs Standard + + @Override + @Nonnull + protected DriverTypeEnum.ConnectionProperties getConnectionProperties() { + return ourContainerExtension.getConnectionProperties(); + } +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerEnterpriseCollectedMigrationTests.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerEnterpriseCollectedMigrationTests.java new file mode 100644 index 00000000000..586f4622cfb --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerEnterpriseCollectedMigrationTests.java @@ -0,0 +1,22 @@ +package ca.uhn.fhir.jpa.migrate.taskdef.containertests; + +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.testcontainers.containers.MSSQLServerContainer; + +import jakarta.annotation.Nonnull; + +public class SqlServerEnterpriseCollectedMigrationTests extends BaseCollectedMigrationTaskSuite { + @RegisterExtension + static TestContainerDatabaseMigrationExtension ourContainerExtension = + new TestContainerDatabaseMigrationExtension( + DriverTypeEnum.MSSQL_2012, + new MSSQLServerContainer<>("mcr.microsoft.com/mssql/server:2019-latest") + .withEnv("ACCEPT_EULA", "Y") + .withEnv("MSSQL_PID", "Enterprise")); // Product id: Sql Server Enterprise vs Standard vs Developer vs ???? + + @Override + @Nonnull + protected DriverTypeEnum.ConnectionProperties getConnectionProperties() { + return ourContainerExtension.getConnectionProperties(); + }} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerStandardCollectedMigrationTest.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerStandardCollectedMigrationTest.java new file mode 100644 index 00000000000..2ac064edae9 --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/SqlServerStandardCollectedMigrationTest.java @@ -0,0 +1,28 @@ +package ca.uhn.fhir.jpa.migrate.taskdef.containertests; + +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.testcontainers.containers.MSSQLServerContainer; +import org.testcontainers.junit.jupiter.Testcontainers; + +import jakarta.annotation.Nonnull; + +@Testcontainers(disabledWithoutDocker=true) +public class SqlServerStandardCollectedMigrationTest extends BaseCollectedMigrationTaskSuite { + + @RegisterExtension + static TestContainerDatabaseMigrationExtension ourContainerExtension = + new TestContainerDatabaseMigrationExtension( + DriverTypeEnum.MSSQL_2012, + new MSSQLServerContainer<>("mcr.microsoft.com/mssql/server:2019-latest") + .withEnv("ACCEPT_EULA", "Y") + .withEnv("MSSQL_PID", "Standard") // Product id: Sql Server Enterprise vs Standard vs Developer vs ???? + ); + + @Override + @Nonnull + protected DriverTypeEnum.ConnectionProperties getConnectionProperties() { + return ourContainerExtension.getConnectionProperties(); + } + +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/TestContainerDatabaseMigrationExtension.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/TestContainerDatabaseMigrationExtension.java new file mode 100644 index 00000000000..51cb254bcf0 --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/TestContainerDatabaseMigrationExtension.java @@ -0,0 +1,50 @@ +package ca.uhn.fhir.jpa.migrate.taskdef.containertests; + +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; +import org.apache.commons.lang3.RandomStringUtils; +import org.junit.jupiter.api.extension.AfterAllCallback; +import org.junit.jupiter.api.extension.BeforeAllCallback; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testcontainers.containers.JdbcDatabaseContainer; + +import jakarta.annotation.Nonnull; + +/** + * Starts a database from TestContainers, and exposes ConnectionProperties for the migrator. + */ +public class TestContainerDatabaseMigrationExtension implements BeforeAllCallback, AfterAllCallback { + private static final Logger ourLog = LoggerFactory.getLogger(TestContainerDatabaseMigrationExtension.class); + + final JdbcDatabaseContainer myJdbcDatabaseContainer; + final DriverTypeEnum myDriverTypeEnum; + + public TestContainerDatabaseMigrationExtension( + DriverTypeEnum theDriverTypeEnum, + JdbcDatabaseContainer theJdbcDatabaseContainer) { + myDriverTypeEnum = theDriverTypeEnum; + myJdbcDatabaseContainer = theJdbcDatabaseContainer + // use a random password to avoid having open ports on hard-coded passwords + .withPassword("!@Aa" + RandomStringUtils.randomAlphanumeric(20)); + } + + @Override + public void beforeAll(ExtensionContext context) { + ourLog.info("Starting container {}", myJdbcDatabaseContainer.getContainerInfo()); + myJdbcDatabaseContainer.start(); + } + + @Override + public void afterAll(ExtensionContext context) { + ourLog.info("Stopping container {}", myJdbcDatabaseContainer.getContainerInfo()); + myJdbcDatabaseContainer.stop(); + } + + + @Nonnull + public DriverTypeEnum.ConnectionProperties getConnectionProperties() { + return myDriverTypeEnum.newConnectionProperties(myJdbcDatabaseContainer.getJdbcUrl(), myJdbcDatabaseContainer.getUsername(), myJdbcDatabaseContainer.getPassword()); + } + +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/package-info.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/package-info.java new file mode 100644 index 00000000000..4050bf1491f --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/containertests/package-info.java @@ -0,0 +1,4 @@ +/** + * Collection of integration tests of migration tasks against real databases. + */ +package ca.uhn.fhir.jpa.migrate.taskdef.containertests;