Avoid unsupported features during migrations on MSSQL (#3652)

Avoid unsupported ONLINE index operations on MSSQL Standard

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() {}

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."

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<String> myColumns;
 	private Boolean myUnique;
 	private List<String> 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 (";
+			mssqlWhereClause = buildMSSqlNotNullWhereClause();
-			for (int i = 0; i < myColumns.size(); i++) {
-				mssqlWhereClause += myColumns.get(i) + " IS NOT NULL ";
-				if (i < myColumns.size() - 1) {
-					mssqlWhereClause += "AND ";
-				}
-			}
-			mssqlWhereClause += ")";
 		}
+		// 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;
+	}
 }

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;
+	}
+}

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);
+				}
+			}
 		}
 	}
 }

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));
+	}
+
+}