From bce031345828a4ab198bdc91b000a219dfd90c3f Mon Sep 17 00:00:00 2001 From: Tadgh Date: Fri, 21 Jun 2024 17:36:00 -0700 Subject: [PATCH] 6034 poor index generation on sql server (#6035) * Changelog * Remove uniqueness * spotless * Add migrations * spotless * make some space * Handle fancier upgrade * spotless * Fix up where clause generation to be a bit more readable * spotless --- .../7_4_0/6034-sql-server-index-creation.yaml | 4 +++ .../tasks/HapiFhirJpaMigrationTasks.java | 32 +++++++++++++++++++ .../entity/ResourceIndexedSearchParamUri.java | 7 ++-- .../jpa/migrate/taskdef/AddIndexTask.java | 29 +++++++++++++---- .../fhir/jpa/migrate/tasks/api/Builder.java | 25 +++++++++++++++ .../migrate/tasks/api/ColumnAndNullable.java | 22 +++++++++++++ 6 files changed, 107 insertions(+), 12 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6034-sql-server-index-creation.yaml create mode 100644 hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/ColumnAndNullable.java diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6034-sql-server-index-creation.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6034-sql-server-index-creation.yaml new file mode 100644 index 00000000000..3377deb1420 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6034-sql-server-index-creation.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 6034 +title: "Two indexes introduced in HAPI-FHIR 6.6.0, `IDX_SP_URI_HASH_IDENTITY_V2` and `IDX_SP_URI_HASH_URI_V2` were previously created as unique indexes. This has caused issues on SQL Server due to the way that a filtered index is created. The unique clause was not necessary to this index, and has been removed." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java index ae4e9f5f9a0..10b58c19b22 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java @@ -32,6 +32,7 @@ import ca.uhn.fhir.jpa.migrate.taskdef.ForceIdMigrationCopyTask; import ca.uhn.fhir.jpa.migrate.taskdef.ForceIdMigrationFixTask; import ca.uhn.fhir.jpa.migrate.tasks.api.BaseMigrationTasks; import ca.uhn.fhir.jpa.migrate.tasks.api.Builder; +import ca.uhn.fhir.jpa.migrate.tasks.api.ColumnAndNullable; import ca.uhn.fhir.jpa.migrate.tasks.api.TaskFlagEnum; import ca.uhn.fhir.jpa.model.config.PartitionSettings; import ca.uhn.fhir.jpa.model.entity.BaseResourceIndexedSearchParam; @@ -348,6 +349,37 @@ public class HapiFhirJpaMigrationTasks extends BaseMigrationTasks { .withType(ColumnTypeEnum.STRING, 100) .failureAllowed(); } + + { + // Note that these are recreations of a previous migration from 6.6.0. The original migration had these set + // as unique, + // which causes SQL Server to create a filtered index. See + // https://www.sqlshack.com/introduction-to-sql-server-filtered-indexes/ + // What this means for hibernate search is that for any column that is nullable, the SQLServerDialect will + // omit the whole row from the index if + // the value of the nullable column is actually null. Removing the uniqueness constraint works around this + // problem. + Builder.BuilderWithTableName uriTable = version.onTable("HFJ_SPIDX_URI"); + + uriTable.dropIndex("20240620.10", "IDX_SP_URI_HASH_URI_V2"); + uriTable.dropIndex("20240620.20", "IDX_SP_URI_HASH_IDENTITY_V2"); + + uriTable.addIndex("20240620.30", "IDX_SP_URI_HASH_URI_V2") + .unique(false) + .online(true) + .withPossibleNullableColumns( + new ColumnAndNullable("HASH_URI", true), + new ColumnAndNullable("RES_ID", false), + new ColumnAndNullable("PARTITION_ID", true)); + uriTable.addIndex("20240620.40", "IDX_SP_URI_HASH_IDENTITY_V2") + .unique(false) + .online(true) + .withPossibleNullableColumns( + new ColumnAndNullable("HASH_IDENTITY", true), + new ColumnAndNullable("SP_URI", true), + new ColumnAndNullable("RES_ID", false), + new ColumnAndNullable("PARTITION_ID", true)); + } } protected void init720() { diff --git a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamUri.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamUri.java index 02a5f23a16f..f7fb75439a8 100644 --- a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamUri.java +++ b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamUri.java @@ -54,12 +54,9 @@ import static org.apache.commons.lang3.StringUtils.defaultString; name = "HFJ_SPIDX_URI", indexes = { // for queries - @Index(name = "IDX_SP_URI_HASH_URI_V2", columnList = "HASH_URI,RES_ID,PARTITION_ID", unique = true), + @Index(name = "IDX_SP_URI_HASH_URI_V2", columnList = "HASH_URI,RES_ID,PARTITION_ID"), // for sorting - @Index( - name = "IDX_SP_URI_HASH_IDENTITY_V2", - columnList = "HASH_IDENTITY,SP_URI,RES_ID,PARTITION_ID", - unique = true), + @Index(name = "IDX_SP_URI_HASH_IDENTITY_V2", columnList = "HASH_IDENTITY,SP_URI,RES_ID,PARTITION_ID"), // for index create/delete @Index(name = "IDX_SP_URI_COORDS", columnList = "RES_ID") }) 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 e7e3b079e2b..2251e19b2a8 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 @@ -34,6 +34,7 @@ import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Set; +import java.util.stream.Collectors; public class AddIndexTask extends BaseTableTask { @@ -41,6 +42,7 @@ public class AddIndexTask extends BaseTableTask { private String myIndexName; private List myColumns; + private List myNullableColumns; private Boolean myUnique; private List myIncludeColumns = Collections.emptyList(); /** Should the operation avoid taking a lock on the table */ @@ -64,6 +66,14 @@ public class AddIndexTask extends BaseTableTask { myUnique = theUnique; } + public List getNullableColumns() { + return myNullableColumns; + } + + public void setNullableColumns(List theNullableColumns) { + this.myNullableColumns = theNullableColumns; + } + @Override public void validate() { super.validate(); @@ -171,14 +181,15 @@ public class AddIndexTask extends BaseTableTask { @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 "; - } + String mssqlWhereClause = ""; + if (myNullableColumns == null || myNullableColumns.isEmpty()) { + return mssqlWhereClause; } + + mssqlWhereClause = " WHERE ("; + mssqlWhereClause += myNullableColumns.stream() + .map(column -> column + " IS NOT NULL ") + .collect(Collectors.joining("AND")); mssqlWhereClause += ")"; return mssqlWhereClause; } @@ -187,6 +198,10 @@ public class AddIndexTask extends BaseTableTask { setColumns(Arrays.asList(theColumns)); } + public void setNullableColumns(String... theColumns) { + setNullableColumns(Arrays.asList(theColumns)); + } + public void setIncludeColumns(String... theIncludeColumns) { setIncludeColumns(Arrays.asList(theIncludeColumns)); } diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java index 7639e486a8d..3f5657fba18 100644 --- a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java @@ -405,6 +405,31 @@ public class Builder { return new BuilderCompleteTask(task); } + /** + * THis is strictly needed for SQL Server, as it will create filtered indexes on nullable columns, and we have to build a tail clause which matches what the SQL Server Hibernate dialect does. + */ + public BuilderCompleteTask withPossibleNullableColumns(ColumnAndNullable... theColumns) { + String[] columnNames = Arrays.stream(theColumns) + .map(ColumnAndNullable::getColumnName) + .toArray(String[]::new); + String[] nullableColumnNames = Arrays.stream(theColumns) + .filter(ColumnAndNullable::isNullable) + .map(ColumnAndNullable::getColumnName) + .toArray(String[]::new); + AddIndexTask task = new AddIndexTask(myRelease, myVersion); + task.setTableName(myTableName); + task.setIndexName(myIndexName); + task.setUnique(myUnique); + task.setColumns(columnNames); + task.setNullableColumns(nullableColumnNames); + task.setOnline(myOnline); + if (myIncludeColumns != null) { + task.setIncludeColumns(myIncludeColumns); + } + addTask(task); + return new BuilderCompleteTask(task); + } + public BuilderAddIndexUnique includeColumns(String... theIncludeColumns) { myIncludeColumns = theIncludeColumns; return this; diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/ColumnAndNullable.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/ColumnAndNullable.java new file mode 100644 index 00000000000..ea89dccfd0e --- /dev/null +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/ColumnAndNullable.java @@ -0,0 +1,22 @@ +package ca.uhn.fhir.jpa.migrate.tasks.api; + +/** + * Simple data class for holding information about a column, and whether it was nullable at time of writing this migration. + */ +public class ColumnAndNullable { + private final String myColumnName; + private final boolean myNullable; + + public ColumnAndNullable(String myColumnName, boolean myNullable) { + this.myColumnName = myColumnName; + this.myNullable = myNullable; + } + + public String getColumnName() { + return myColumnName; + } + + public boolean isNullable() { + return myNullable; + } +}