Drop tag definition constraint

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6582-drop-tag-fk-constraint.yaml

@@ -0,0 +1,4 @@
+---
+type: perf
+issue: 6582
+title: "Under heavy load, a foreign key constraint in the Tag Definition table (used for Tags, Security Labels, and Profile Definitions) can cause serious slowdowns when writing large numbers of resources (particularly if many resources contain the same tags/labels, or if the resources are being written individually or in smaller batches). This has been corrected."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java

@@ -671,17 +671,17 @@ private boolean updateTags(
 
 		allResourceTagsNewAndOldFromTheEntity.forEach(tag -> {
 
-			// Don't keep duplicate tags
-			if (!allTagDefinitionsPresent.add(tag.getTag())) {
+			// Don't keep duplicate tags or tags with a missing definition
+			TagDefinition tagDefinition = tag.getTag();
+			if (tagDefinition == null || !allTagDefinitionsPresent.add(tagDefinition)) {
 				theEntity.getTags().remove(tag);
 			}
 
 			// Drop any tags that have been removed
-			if (!allResourceTagsFromTheResource.contains(tag)) {
+			if (tagDefinition != null && !allResourceTagsFromTheResource.contains(tag)) {
 				if (shouldDroppedTagBeRemovedOnUpdate(theRequest, tag)) {
 					theEntity.getTags().remove(tag);
-				} else if (HapiExtensions.EXT_SUBSCRIPTION_MATCHING_STRATEGY.equals(
-						tag.getTag().getSystem())) {
+				} else if (HapiExtensions.EXT_SUBSCRIPTION_MATCHING_STRATEGY.equals(tagDefinition.getSystem())) {
 					theEntity.getTags().remove(tag);
 				}
 			}
@@ -766,6 +766,9 @@ public BaseHasResource readEntity(IIdType theValueId, RequestDetails theRequest)
 	 * @return Returns <code>true</code> if the tag should be removed
 	 */
 	protected boolean shouldDroppedTagBeRemovedOnUpdate(RequestDetails theRequest, ResourceTag theTag) {
+		if (theTag.getTag() == null) {
+			return true;
+		}
 
 		Set<TagTypeEnum> metaSnapshotModeTokens = null;
 

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java

@@ -490,7 +490,19 @@ private <R extends IBaseResource> R populateResourceMetadataRi(
 			res.getMeta().getTag().clear();
 			res.getMeta().getProfile().clear();
 			res.getMeta().getSecurity().clear();
+
+			boolean haveWarnedForMissingTag = false;
 			for (BaseTag next : theTagList) {
+				if (next.getTag() == null) {
+					if (!haveWarnedForMissingTag) {
+						ourLog.warn(
+								"Tag definition HFJ_TAG_DEF#{} is missing, returned Resource.meta may not be complete",
+								next.getTagId());
+						haveWarnedForMissingTag = true;
+					}
+					continue;
+				}
+
 				switch (next.getTag().getTagType()) {
 					case PROFILE:
 						res.getMeta().addProfile(next.getTag().getCode());

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java

@@ -379,6 +379,13 @@ protected void init780() {
 				.nullable()
 				.withType(ColumnTypeEnum.TINYINT)
 				.heavyweightSkipByDefault();
+
+		/*
+		 * These two constraints were the last two that we had that used
+		 * hibernate-generated names. Yay!
+		 */
+		version.onTable("HFJ_RES_TAG").dropForeignKey("20250102.10", "FKbfcjbaftmiwr3rxkwsy23vneo", "HFJ_TAG_DEF");
+		version.onTable("HFJ_HISTORY_TAG").dropForeignKey("20250102.20", "FKtderym7awj6q8iq5c51xv4ndw", "HFJ_TAG_DEF");
 	}
 
 	protected void init780_afterPartitionChanges() {

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BaseTag.java

@@ -19,10 +19,15 @@
  */
 package ca.uhn.fhir.jpa.model.entity;
 
+import jakarta.annotation.Nullable;
 import jakarta.persistence.Column;
+import jakarta.persistence.ConstraintMode;
+import jakarta.persistence.ForeignKey;
 import jakarta.persistence.JoinColumn;
 import jakarta.persistence.ManyToOne;
 import jakarta.persistence.MappedSuperclass;
+import org.hibernate.annotations.NotFound;
+import org.hibernate.annotations.NotFoundAction;
 
 import java.io.Serializable;
 
@@ -31,9 +36,15 @@ public abstract class BaseTag extends BasePartitionable implements Serializable
 
 	private static final long serialVersionUID = 1L;
 
-	// many baseTags -> one tag definition
+	/**
+	 * Every tag has a reference to the tag definition. Note that this field
+	 * must not have a FK constraint! In this case, Postgres (and maybe others)
+	 * are horribly slow writing to the table if there's an FK constraint.
+	 * See https://pganalyze.com/blog/5mins-postgres-multiXact-ids-foreign-keys-performance
+	 */
 	@ManyToOne(cascade = {})
-	@JoinColumn(name = "TAG_ID", nullable = false)
+	@JoinColumn(name = "TAG_ID", nullable = false, foreignKey = @ForeignKey(ConstraintMode.NO_CONSTRAINT))
+	@NotFound(action = NotFoundAction.IGNORE)
 	private TagDefinition myTag;
 
 	@Column(name = "TAG_ID", insertable = false, updatable = false)
@@ -43,6 +54,12 @@ public Long getTagId() {
 		return myTagId;
 	}
 
+	/**
+	 * This can be null if the tag definition has been deleted. This
+	 * should never happen unless someone has manually messed with
+	 * the database, but it could happen.
+	 */
+	@Nullable
 	public TagDefinition getTag() {
 		return myTag;
 	}

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceHistoryTag.java

@@ -154,12 +154,12 @@ public JpaPid getResourcePid() {
 	public String toString() {
 		ToStringBuilder b = new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE);
 		b.append("id", getId());
-		if (getPartitionId() != null) {
+		if (getPartitionId().getPartitionId() != null) {
 			b.append("partId", getPartitionId().getPartitionId());
 		}
 		b.append("versionId", myResourceHistoryPid);
 		b.append("resId", getResourceId());
-		b.append("tag", getTag().getId());
+		b.append("tag", getTagId());
 		return b.build();
 	}
 }

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceTag.java

@@ -175,7 +175,7 @@ public String toString() {
 			b.append("partition", getPartitionId().getPartitionId());
 		}
 		b.append("resId", getResourceId());
-		b.append("tag", getTag().getId());
+		b.append("tag", getTagId());
 		return b.build();
 	}
 }

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java

@@ -2,12 +2,17 @@
 
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
+import ca.uhn.fhir.jpa.dao.JpaStorageResourceParser;
 import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
 import ca.uhn.fhir.rest.gclient.TokenClientParam;
 import ca.uhn.fhir.util.BundleBuilder;
+import ca.uhn.test.util.LogbackTestExtension;
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import jakarta.annotation.Nonnull;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.Bundle;
 import org.hl7.fhir.r4.model.Enumerations;
@@ -17,8 +22,11 @@
 import org.hl7.fhir.r4.model.SearchParameter;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.ValueSource;
 
-import jakarta.annotation.Nonnull;
 import java.util.List;
 import java.util.stream.Collectors;
 
@@ -32,6 +40,9 @@ public class FhirResourceDaoR4TagsTest extends BaseResourceProviderR4Test {
 
 	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoR4TagsTest.class);
 
+	@RegisterExtension
+	private LogbackTestExtension myLogbackTestExtension = new LogbackTestExtension(JpaStorageResourceParser.class, Level.WARN);
+
 	@Override
 	@AfterEach
 	public final void after() throws Exception {
@@ -182,6 +193,83 @@ public void testDeleteResourceWithTags_NonVersionedTags_InTransaction() {
 
 	}
 
+	/**
+	 * We have removed the FK constraint from {@link ca.uhn.fhir.jpa.model.entity.ResourceTag}
+	 * to {@link ca.uhn.fhir.jpa.model.entity.TagDefinition} so it's possible that tag
+	 * definitions get deleted. This test makes sure we act sanely when that happens.
+	 */
+	@ParameterizedTest
+	@CsvSource(value = {
+		"NON_VERSIONED, read",
+		"NON_VERSIONED, vread",
+		"NON_VERSIONED, search",
+		"NON_VERSIONED, history",
+		"NON_VERSIONED, update",
+		"VERSIONED,     read",
+		"VERSIONED,     vread",
+		"VERSIONED,     search",
+		"VERSIONED,     history",
+		"VERSIONED,     update"
+	})
+	public void testTagDefinitionDeleted(JpaStorageSettings.TagStorageModeEnum theTagStorageMode, String theOperation) {
+		// Setup
+		myStorageSettings.setTagStorageMode(theTagStorageMode);
+
+		Patient patient = new Patient();
+		patient.setId("Patient/A");
+		patient.getMeta().addProfile("http://profile0");
+		patient.getMeta().addProfile("http://profile1");
+		patient.getMeta().addTag("http://tag", "tag0", "display0");
+		patient.getMeta().addTag("http://tag", "tag1", "display1");
+		patient.getMeta().addSecurity("http://security", "security0", "display0");
+		patient.getMeta().addSecurity("http://security", "security1", "display1");
+		myPatientDao.update(patient, mySrd);
+
+		patient.setActive(false);
+		myPatientDao.update(patient, mySrd);
+
+		// Test
+
+		runInTransaction(() -> {
+			assertEquals(6, myTagDefinitionDao.count());
+			myTagDefinitionDao.deleteAll();
+			assertEquals(0, myTagDefinitionDao.count());
+		});
+		Patient actualPatient;
+		switch (theOperation) {
+			case "read":
+				actualPatient = myPatientDao.read(new IdType("Patient/A"), mySrd);
+				break;
+			case "vread":
+				actualPatient = myPatientDao.read(new IdType("Patient/A/_history/1"), mySrd);
+				break;
+			case "search":
+				actualPatient = (Patient) myPatientDao.search(SearchParameterMap.newSynchronous(), mySrd).getResources(0, 1).get(0);
+				break;
+			case "history":
+				actualPatient = (Patient) mySystemDao.history(null, null, null, mySrd).getResources(0, 1).get(0);
+				break;
+			case "update":
+				Patient updatePatient = new Patient();
+				updatePatient.setId("Patient/A");
+				updatePatient.setActive(true);
+				actualPatient = (Patient) myPatientDao.update(updatePatient, mySrd).getResource();
+				break;
+			default:
+				throw new IllegalArgumentException("Unknown operation: " + theOperation);
+		}
+
+		// Verify
+
+		assertEquals(0, actualPatient.getMeta().getProfile().size());
+		assertEquals(0, actualPatient.getMeta().getTag().size());
+		assertEquals(0, actualPatient.getMeta().getSecurity().size());
+
+		List<ILoggingEvent> logEvents = myLogbackTestExtension.getLogEvents(t -> t.getMessage().contains("Tag definition HFJ_TAG_DEF#{} is missing"));
+		assertThat(logEvents).as(() -> myLogbackTestExtension.getLogEvents().toString()).hasSize(1);
+	}
+
+
 	/**
 	 * Make sure tags are preserved
 	 */
@@ -510,21 +598,6 @@ public void testInlineTags_Search_Security() {
 		validatePatientSearchResultsForInlineTags(outcome);
 	}
 
-	@Nonnull
-	public static SearchParameter createSecuritySearchParameter(FhirContext fhirContext) {
-		SearchParameter searchParameter = new SearchParameter();
-		searchParameter.setId("SearchParameter/resource-security");
-		for (String next : fhirContext.getResourceTypes().stream().sorted().collect(Collectors.toList())) {
-			searchParameter.addBase(next);
-		}
-		searchParameter.setStatus(Enumerations.PublicationStatus.ACTIVE);
-		searchParameter.setType(Enumerations.SearchParamType.TOKEN);
-		searchParameter.setCode("_security");
-		searchParameter.setName("Security");
-		searchParameter.setExpression("meta.security");
-		return searchParameter;
-	}
-
 	private void validatePatientSearchResultsForInlineTags(Bundle outcome) {
 		Patient patient;
 		patient = (Patient) outcome.getEntry().get(0).getResource();
@@ -599,6 +672,21 @@ private void initializeVersioned() {
 		assertEquals("2", myPatientDao.update(patient, mySrd).getId().getVersionIdPart());
 	}
 
+	@Nonnull
+	public static SearchParameter createSecuritySearchParameter(FhirContext fhirContext) {
+		SearchParameter searchParameter = new SearchParameter();
+		searchParameter.setId("SearchParameter/resource-security");
+		for (String next : fhirContext.getResourceTypes().stream().sorted().collect(Collectors.toList())) {
+			searchParameter.addBase(next);
+		}
+		searchParameter.setStatus(Enumerations.PublicationStatus.ACTIVE);
+		searchParameter.setType(Enumerations.SearchParamType.TOKEN);
+		searchParameter.setCode("_security");
+		searchParameter.setName("Security");
+		searchParameter.setExpression("meta.security");
+		return searchParameter;
+	}
+
 	@Nonnull
 	static List<String> toTags(Patient patient) {
 		return toTags(patient.getMeta());

hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java

@@ -276,7 +276,8 @@ public static ColumnType getColumnType(
 	}
 
 	/**
-	 * Retrieve all index names
+	 * Retrieve all index names. The returned names will be in upper case
+	 * always.
 	 */
 	public static Set<String> getForeignKeys(
 			DriverTypeEnum.ConnectionProperties theConnectionProperties,
@@ -588,9 +589,9 @@ public static boolean isColumnNullable(
 		}
 	}
 
-	private static String massageIdentifier(DatabaseMetaData theMetadata, String theCatalog) throws SQLException {
-		String retVal = theCatalog;
-		if (theCatalog == null) {
+	public static String massageIdentifier(DatabaseMetaData theMetadata, String theIdentifier) throws SQLException {
+		String retVal = theIdentifier;
+		if (theIdentifier == null) {
 			return null;
 		} else if (theMetadata.storesLowerCaseIdentifiers()) {
 			retVal = retVal.toLowerCase();

hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropForeignKeyTask.java

@@ -26,12 +26,15 @@
 import org.apache.commons.lang3.Validate;
 import org.apache.commons.lang3.builder.EqualsBuilder;
 import org.apache.commons.lang3.builder.HashCodeBuilder;
+import org.intellij.lang.annotations.Language;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.sql.Connection;
 import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Locale;
 import java.util.Set;
 
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
@@ -92,14 +95,18 @@ public void validate() {
 	public void doExecute() throws SQLException {
 
 		Set<String> existing = JdbcUtils.getForeignKeys(getConnectionProperties(), myParentTableName, getTableName());
-		if (!existing.contains(myConstraintName)) {
+		if (!existing.contains(myConstraintName.toUpperCase(Locale.US))) {
 			logInfo(ourLog, "Don't have constraint named {} - No action performed", myConstraintName);
 			return;
 		}
 
-		List<String> sqls = generateSql(getTableName(), myConstraintName, getDriverType());
+		List<String> sqlStatements;
+		try (Connection connection = getConnectionProperties().getDataSource().getConnection()) {
+			String constraintName = JdbcUtils.massageIdentifier(connection.getMetaData(), myConstraintName);
+			sqlStatements = generateSql(getTableName(), constraintName, getDriverType());
+		}
 
-		for (String next : sqls) {
+		for (@Language("SQL") String next : sqlStatements) {
 			executeSql(getTableName(), next);
 		}
 	}

pom.xml

@@ -2528,6 +2528,11 @@
 					<artifactId>maven-dependency-plugin</artifactId>
 					<version>3.8.1</version>
 				</plugin>
+				<plugin>
+					<groupId>org.apache.maven.plugins</groupId>
+					<artifactId>maven-deploy-plugin</artifactId>
+					<version>3.1.3</version>
+				</plugin>
 				<plugin>
 					<groupId>org.sonatype.plugins</groupId>
 					<artifactId>nexus-staging-maven-plugin</artifactId>