Expunge performance tweak (#4920)

* Expunge performance tweak

* Add changelog

* Address review comment

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4920-tiny-expunge-perf-tweak.yaml

@@ -0,0 +1,5 @@
+---
+type: perf
+issue: 4920
+title: "The `$expunge` operation has been slightly optimized and should issue fewer SQL
+  statements to the database."

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

@@ -145,4 +145,7 @@ public interface IResourceTableDao extends JpaRepository<ResourceTable, Long>, I
 
 	@Query("SELECT t FROM ResourceTable t LEFT JOIN FETCH t.myForcedId WHERE t.myPartitionId.myPartitionId IN (:partitionIds) AND t.myId = :pid")
 	Optional<ResourceTable> readByPartitionIds(@Param("partitionIds") Collection<Integer> thrValues, @Param("pid") Long theResourceId);
+
+	@Query("SELECT t FROM ResourceTable t LEFT JOIN FETCH t.myForcedId WHERE t.myId IN :pids")
+	List<ResourceTable> findAllByIdAndLoadForcedIds(@Param("pids") List<Long> thePids);
 }

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

@@ -236,8 +236,11 @@ public class JpaResourceExpungeService implements IResourceExpungeService<JpaPid
 	@Override
 	@Transactional
 	public void expungeHistoricalVersionsOfIds(RequestDetails theRequestDetails, List<JpaPid> theResourceIds, AtomicInteger theRemainingCount) {
-		for (JpaPid next : theResourceIds) {
+		List<Long> pids = JpaPid.toLongList(theResourceIds);
-			expungeHistoricalVersionsOfId(theRequestDetails, (next).getId(), theRemainingCount);
+
+		List<ResourceTable> resourcesToDelete = myResourceTableDao.findAllByIdAndLoadForcedIds(pids);
+		for (ResourceTable next : resourcesToDelete) {
+			expungeHistoricalVersionsOfId(theRequestDetails, next, theRemainingCount);
 			if (expungeLimitReached(theRemainingCount)) {
 				return;
 			}
@@ -267,12 +270,12 @@ public class JpaResourceExpungeService implements IResourceExpungeService<JpaPid
 
 		deleteAllSearchParams(JpaPid.fromId(resource.getResourceId()));
 
-		myResourceTagDao.deleteByResourceId(resource.getId());
+		if (resource.isHasTags()) {
+			myResourceTagDao.deleteByResourceId(resource.getId());
+		}
 
 		if (resource.getForcedId() != null) {
 			ForcedId forcedId = resource.getForcedId();
-			resource.setForcedId(null);
-			myResourceTableDao.saveAndFlush(resource);
 			myForcedIdDao.deleteByPid(forcedId.getId());
 		}
 
@@ -323,7 +326,7 @@ public class JpaResourceExpungeService implements IResourceExpungeService<JpaPid
 		}
 	}
 
-	private void expungeHistoricalVersionsOfId(RequestDetails theRequestDetails, Long myResourceId, AtomicInteger theRemainingCount) {
+	private void expungeHistoricalVersionsOfId(RequestDetails theRequestDetails, ResourceTable theResource, AtomicInteger theRemainingCount) {
 		Pageable page;
 		synchronized (theRemainingCount) {
 			if (expungeLimitReached(theRemainingCount)) {
@@ -332,10 +335,8 @@ public class JpaResourceExpungeService implements IResourceExpungeService<JpaPid
 			page = PageRequest.of(0, theRemainingCount.get());
 		}
 
-		ResourceTable resource = myResourceTableDao.findById(myResourceId).orElseThrow(IllegalArgumentException::new);
+		Slice<Long> versionIds = myResourceHistoryTableDao.findForResourceId(page, theResource.getId(), theResource.getVersion());
-
+		ourLog.debug("Found {} versions of resource {} to expunge", versionIds.getNumberOfElements(), theResource.getIdDt().getValue());
-		Slice<Long> versionIds = myResourceHistoryTableDao.findForResourceId(page, resource.getId(), resource.getVersion());
-		ourLog.debug("Found {} versions of resource {} to expunge", versionIds.getNumberOfElements(), resource.getIdDt().getValue());
 		for (Long nextVersionId : versionIds) {
 			expungeHistoricalVersion(theRequestDetails, nextVersionId, theRemainingCount);
 			if (expungeLimitReached(theRemainingCount)) {

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

@@ -48,6 +48,7 @@ import org.hibernate.search.mapper.pojo.mapping.definition.annotation.PropertyBi
 import org.hibernate.search.mapper.pojo.mapping.definition.annotation.PropertyValue;
 import org.hibernate.tuple.ValueGenerator;
 import org.hl7.fhir.instance.model.api.IIdType;
+import org.hl7.fhir.r4.model.InstantType;
 
 import javax.persistence.CascadeType;
 import javax.persistence.Column;
@@ -818,7 +819,7 @@ public class ResourceTable extends BaseHasResource implements Serializable, IBas
 		}
 		b.append("lastUpdated", getUpdated().getValueAsString());
 		if (getDeleted() != null) {
-			b.append("deleted");
+			b.append("deleted", new InstantType(getDeleted()).getValueAsString());
 		}
 		return b.build();
 	}

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

@@ -14,6 +14,7 @@ import ca.uhn.fhir.context.support.ValueSetExpansionOptions;
 import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
 import ca.uhn.fhir.jpa.api.dao.ReindexParameters;
 import ca.uhn.fhir.jpa.api.model.DeleteMethodOutcome;
+import ca.uhn.fhir.jpa.api.model.ExpungeOptions;
 import ca.uhn.fhir.jpa.api.model.HistoryCountModeEnum;
 import ca.uhn.fhir.jpa.dao.data.ISearchParamPresentDao;
 import ca.uhn.fhir.jpa.entity.TermValueSet;
@@ -100,6 +101,7 @@ import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.not;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -176,6 +178,61 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
 		myValidationSupport.fetchAllStructureDefinitions();
 	}
 
+	/**
+	 * See the class javadoc before changing the counts in this test!
+	 */
+	@Test
+	public void testExpungeAllVersionsWithTagsDeletesRow() {
+		// Setup
+		// Create then delete
+		for (int i = 0; i < 5; i++) {
+			Patient p = new Patient();
+			p.setId("TEST" + i);
+			p.getMeta().addTag().setSystem("http://foo").setCode("bar");
+			p.setActive(true);
+			p.addName().setFamily("FOO");
+			myPatientDao.update(p).getId();
+
+			for (int j = 0; j < 5; j++) {
+				p.setActive(!p.getActive());
+				myPatientDao.update(p);
+			}
+
+			myPatientDao.delete(new IdType("Patient/TEST" + i));
+		}
+
+
+		runInTransaction(() -> assertThat(myResourceTableDao.findAll(), not(empty())));
+		runInTransaction(() -> assertThat(myResourceHistoryTableDao.findAll(), not(empty())));
+		runInTransaction(() -> assertThat(myForcedIdDao.findAll(), not(empty())));
+
+		logAllResources();
+
+		// Test
+		myCaptureQueriesListener.clear();
+		myPatientDao.expunge(new ExpungeOptions()
+			.setExpungeDeletedResources(true), null);
+
+		// Verify
+		/*
+		 * Note: $expunge is still pretty inefficient. We load all the HFJ_RESOURCE entities
+		 * in one shot, but we then load HFJ_RES_VER entities one by one and delete the FK
+		 * constraints on both HFJ_RESOURCE and HFJ_RES_VER one by one. This could definitely
+		 * stand to be optimized. The one gotcha is that we call an interceptor for each
+		 * version being deleted (I think so that MDM can do cleanup?) so we need to be careful
+		 * about any batch deletes.
+		 */
+		assertEquals(47, myCaptureQueriesListener.countSelectQueriesForCurrentThread());
+		assertEquals(0, myCaptureQueriesListener.countUpdateQueriesForCurrentThread());
+		assertEquals(0, myCaptureQueriesListener.countInsertQueriesForCurrentThread());
+		assertEquals(85, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
+
+		runInTransaction(() -> assertThat(myResourceTableDao.findAll(), empty()));
+		runInTransaction(() -> assertThat(myResourceHistoryTableDao.findAll(), empty()));
+		runInTransaction(() -> assertThat(myForcedIdDao.findAll(), empty()));
+
+	}
+
 	/**
 	 * See the class javadoc before changing the counts in this test!
 	 */

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ExpungeR4Test.java

@@ -378,6 +378,7 @@ public class ExpungeR4Test extends BaseResourceProviderR4Test {
 
 	@Test
 	public void testExpungeAllVersionsWithTagsDeletesRow() {
+		// Setup
 		// Create then delete
 		Patient p = new Patient();
 		p.setId("TEST");
@@ -395,10 +396,18 @@ public class ExpungeR4Test extends BaseResourceProviderR4Test {
 		runInTransaction(() -> assertThat(myResourceHistoryTableDao.findAll(), not(empty())));
 		runInTransaction(() -> assertThat(myForcedIdDao.findAll(), not(empty())));
 
+		// Test
+		myCaptureQueriesListener.clear();
 		myPatientDao.expunge(new ExpungeOptions()
 			.setExpungeDeletedResources(true)
 			.setExpungeOldVersions(true), null);
 
+		// Verify
+		assertEquals(8, myCaptureQueriesListener.countSelectQueriesForCurrentThread());
+		assertEquals(0, myCaptureQueriesListener.countUpdateQueriesForCurrentThread());
+		assertEquals(0, myCaptureQueriesListener.countInsertQueriesForCurrentThread());
+		assertEquals(9, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
+
 		runInTransaction(() -> assertThat(myResourceTableDao.findAll(), empty()));
 		runInTransaction(() -> assertThat(myResourceHistoryTableDao.findAll(), empty()));
 		runInTransaction(() -> assertThat(myForcedIdDao.findAll(), empty()));