From 92acd9b001b41ee2b2868cebd9840f8edba4526f Mon Sep 17 00:00:00 2001 From: michaelabuckley Date: Tue, 31 May 2022 14:44:42 -0400 Subject: [PATCH] Avoid unsupported features during migrations on MSSQL (#3652) Avoid unsupported ONLINE index operations on MSSQL Standard --- .../src/main/java/ca/uhn/fhir/i18n/Msg.java | 2 +- .../changelog/6_1_0/3651-MSSQL-migration.yaml | 6 +++ .../jpa/migrate/taskdef/AddIndexTask.java | 34 +++++++++---- .../jpa/migrate/taskdef/MetadataSource.java | 44 ++++++++++++++++ ...ddIndexTest.java => AddIndexTaskTest.java} | 32 +++++++++++- .../migrate/taskdef/MetadataSourceTest.java | 50 +++++++++++++++++++ 6 files changed, 156 insertions(+), 12 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_1_0/3651-MSSQL-migration.yaml create mode 100644 hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSource.java rename hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/{AddIndexTest.java => AddIndexTaskTest.java} (83%) create mode 100644 hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSourceTest.java diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/i18n/Msg.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/i18n/Msg.java index c6c960905d9..1add165c850 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/i18n/Msg.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/i18n/Msg.java @@ -25,7 +25,7 @@ public final class Msg { /** * IMPORTANT: Please update the following comment after you add a new code - * Last code value: 2083 + * Last code value: 2084 */ private Msg() {} diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_1_0/3651-MSSQL-migration.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_1_0/3651-MSSQL-migration.yaml new file mode 100644 index 00000000000..6334b6f9b76 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_1_0/3651-MSSQL-migration.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 3651 +jira: SMILE-4394 +title: "MS SQL Standard Edition does not support the ONLINE=ON feature for index creation, and failed during upgrades to 6.0. + Upgrades now detect these limitations and avoid this feature when unsupported." 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 1dc5e52ae57..726e27a39a3 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 @@ -44,8 +44,11 @@ public class AddIndexTask extends BaseTableTask { private List myColumns; private Boolean myUnique; private List myIncludeColumns = Collections.emptyList(); + /** Should the operation avoid taking a lock on the table */ private boolean myOnline; + private MetadataSource myMetadataSource = new MetadataSource(); + public AddIndexTask(String theProductVersion, String theSchemaVersion) { super(theProductVersion, theSchemaVersion); } @@ -120,15 +123,9 @@ public class AddIndexTask extends BaseTableTask { } } if (myUnique && getDriverType() == DriverTypeEnum.MSSQL_2012) { - mssqlWhereClause = " WHERE ("; - for (int i = 0; i < myColumns.size(); i++) { - mssqlWhereClause += myColumns.get(i) + " IS NOT NULL "; - if (i < myColumns.size() - 1) { - mssqlWhereClause += "AND "; - } - } - mssqlWhereClause += ")"; + mssqlWhereClause = buildMSSqlNotNullWhereClause(); } + // Should we do this non-transactionally? Avoids a write-lock, but introduces weird failure modes. String postgresOnline = ""; String oracleOnlineDeferred = ""; if (myOnline) { @@ -143,7 +140,9 @@ public class AddIndexTask extends BaseTableTask { oracleOnlineDeferred = " ONLINE DEFERRED INVALIDATION"; break; case MSSQL_2012: - oracleOnlineDeferred = " WITH (ONLINE = ON)"; + if (myMetadataSource.isOnlineIndexSupported(getConnectionProperties())) { + oracleOnlineDeferred = " WITH (ONLINE = ON)"; + } break; default: } @@ -156,6 +155,20 @@ public class AddIndexTask extends BaseTableTask { return sql; } + @Nonnull + private String buildMSSqlNotNullWhereClause() { + String mssqlWhereClause; + mssqlWhereClause = " WHERE ("; + for (int i = 0; i < myColumns.size(); i++) { + mssqlWhereClause += myColumns.get(i) + " IS NOT NULL "; + if (i < myColumns.size() - 1) { + mssqlWhereClause += "AND "; + } + } + mssqlWhereClause += ")"; + return mssqlWhereClause; + } + public void setColumns(String... theColumns) { setColumns(Arrays.asList(theColumns)); } @@ -197,4 +210,7 @@ public class AddIndexTask extends BaseTableTask { theBuilder.append(myOnline); } + public void setMetadataSource(MetadataSource theMetadataSource) { + myMetadataSource = theMetadataSource; + } } 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 new file mode 100644 index 00000000000..e9860ed69bf --- /dev/null +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSource.java @@ -0,0 +1,44 @@ +package ca.uhn.fhir.jpa.migrate.taskdef; + +import ca.uhn.fhir.i18n.Msg; +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; + +/** + * Helper to extract database information about supported migrations. + */ +public class MetadataSource { + + /** + * Does this database support index operations without write-locking the table? + */ + public boolean isOnlineIndexSupported(DriverTypeEnum.ConnectionProperties theConnectionProperties) { + + switch (theConnectionProperties.getDriverType()) { + case ORACLE_12C: + case POSTGRES_9_4: + case COCKROACHDB_21_1: + return true; + case MSSQL_2012: + String edition = getEdition(theConnectionProperties); + return edition.startsWith("Enterprise"); + default: + return false; + } + } + + /** + * Get the MS Sql Server edition. Other databases are not supported yet. + * + * @param theConnectionProperties the database to inspect + * @return the edition string (e.g. Standard, Enterprise, Developer, etc.) + */ + private String getEdition(DriverTypeEnum.ConnectionProperties theConnectionProperties) { + final String result; + if (theConnectionProperties.getDriverType() == DriverTypeEnum.MSSQL_2012) { + result = theConnectionProperties.newJdbcTemplate().queryForObject("SELECT SERVERPROPERTY ('edition')", String.class); + } else { + throw new UnsupportedOperationException(Msg.code(2084) + "We only know about MSSQL editions."); + } + return result; + } +} diff --git a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTest.java b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskTest.java similarity index 83% rename from hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTest.java rename to hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskTest.java index 41aa010b159..e36f43f819a 100644 --- a/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTest.java +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddIndexTaskTest.java @@ -6,8 +6,14 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; import java.sql.SQLException; import java.util.function.Supplier; @@ -17,8 +23,8 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.hasItem; import static org.junit.jupiter.api.Assertions.assertEquals; -public class AddIndexTest extends BaseTest { - +@MockitoSettings(strictness = Strictness.LENIENT) +public class AddIndexTaskTest extends BaseTest { @ParameterizedTest(name = "{index}: {0}") @MethodSource("data") @@ -149,6 +155,9 @@ public class AddIndexTest extends BaseTest { @Nested public class OnlineNoLocks { + @Mock + private MetadataSource mockMetadataSource; + @BeforeEach public void beforeEach() { myTask = new AddIndexTask("1", "1"); @@ -156,6 +165,7 @@ public class AddIndexTest extends BaseTest { myTask.setTableName("SOMETABLE"); myTask.setColumns("PID", "TEXTCOL"); myTask.setUnique(false); + myTask.setMetadataSource(mockMetadataSource); } @ParameterizedTest(name = "{index}: {0}") @@ -171,6 +181,8 @@ public class AddIndexTest 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) { case POSTGRES_9_4: @@ -189,6 +201,22 @@ public class AddIndexTest extends BaseTest { break; } } + + @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); + } + } } } } 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 new file mode 100644 index 00000000000..3f653e054e4 --- /dev/null +++ b/hapi-fhir-sql-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/MetadataSourceTest.java @@ -0,0 +1,50 @@ +package ca.uhn.fhir.jpa.migrate.taskdef; + +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; +import com.google.common.base.Strings; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; +import org.springframework.jdbc.core.JdbcTemplate; + +import static org.junit.jupiter.api.Assertions.*; + +@MockitoSettings(strictness = Strictness.LENIENT) +class MetadataSourceTest { + MetadataSource myMetadataSource = new MetadataSource(); + @Mock + DriverTypeEnum.ConnectionProperties myConnectionProperties; + @Mock + JdbcTemplate myJdbcTemplate; + + @ParameterizedTest + @CsvSource({ + "H2_EMBEDDED,,false", + "DERBY_EMBEDDED,,false", + "MARIADB_10_1,,false", + "MYSQL_5_7,,false", + "POSTGRES_9_4,,true", + "ORACLE_12C,,true", + "COCKROACHDB_21_1,,true", + // sql server only supports it in Enterprise + // 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,Standard Edition (64-bit),false", + "MSSQL_2012,Enterprise Edition (64-bit),true" + }) + void isOnlineIndexSupported(DriverTypeEnum theType, String theEdition, boolean theSupportedFlag) { + // stub out our Sql Server edition lookup + Mockito.when(myConnectionProperties.getDriverType()).thenReturn(theType); + Mockito.when(myConnectionProperties.newJdbcTemplate()).thenReturn(myJdbcTemplate); + Mockito.when(myJdbcTemplate.queryForObject(Mockito.any(), Mockito.eq(String.class))).thenReturn(Strings.nullToEmpty(theEdition)); + + assertEquals(theSupportedFlag, myMetadataSource.isOnlineIndexSupported(myConnectionProperties)); + } + +}