Issue 4804 full table scan on mpi link during mdm clear (#4805)

* version bump for next release  (#4793)

* version bump

* Bump to correctnumber

* Version Enum and folder

* Remove interim from list

* wip

* Fix operation on nested type-choices in FhirPatch implementation (#4783)

* Fix operation on nested type-choices in FhirPatch implementation

* Add credit for #4783

---------

Co-authored-by: James Agnew <[email protected]>

* #4468 fix previous link offset no cache pagination (#4489)

* #4468 Add test reproducing the issue

* #4468 Fix previous link for no cache offset pagination

* #4468 Use unchecked URI parsing

* Credit for #4489

---------

Co-authored-by: James Agnew <[email protected]>

* Changelog and data generating test

* Add MdmLink index

* Avoid double link deletion

* Use ThreadLocal safely

---------

Co-authored-by: Tadgh <[email protected]>
Co-authored-by: Zach Smith <[email protected]>
Co-authored-by: James Agnew <[email protected]>
Co-authored-by: Aleksej Parovysnik <[email protected]>
Co-authored-by: juan.marchionatto <[email protected]>

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4804-full-table-scan-on-mpi-link-during-mdm-clear.yaml

@@ -0,0 +1,5 @@
+---
+type: fix
+issue: 4804
+jira: SMILE-5145
+title: "Improved performance of `mdm-clear` operation by adding index and avoiding redundant deletion."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/MdmLink.java

@@ -53,11 +53,11 @@
 @Table(name = "MPI_LINK", uniqueConstraints = {
  // TODO GGG DROP this index, and instead use the below one
  @UniqueConstraint(name = "IDX_EMPI_PERSON_TGT", columnNames = {"PERSON_PID", "TARGET_PID"}),
- // v---- this one
- //TODO GGG revisit adding this: @UniqueConstraint(name = "IDX_EMPI_GR_TGT", columnNames = {"GOLDEN_RESOURCE_PID", "TARGET_PID"}),
  //TODO GGG Should i make individual indices for PERSON/TARGET?
 }, indexes = {
- @Index(name = "IDX_EMPI_MATCH_TGT_VER", columnList = "MATCH_RESULT, TARGET_PID, VERSION")
+ @Index(name = "IDX_EMPI_MATCH_TGT_VER", columnList = "MATCH_RESULT, TARGET_PID, VERSION"),
+ // v---- this one
+ @Index(name = "IDX_EMPI_GR_TGT", columnList = "GOLDEN_RESOURCE_PID, TARGET_PID")
 })
 @Audited
 // This is the table name generated by default by envers, but we set it explicitly for clarity

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

@@ -44,6 +44,7 @@
 import ca.uhn.fhir.util.VersionEnum;
 import software.amazon.awssdk.utils.StringUtils;
 
+import javax.persistence.Index;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
@@ -346,6 +347,14 @@ protected void init660() {
  linkTable.addForeignKey("20230424.5", "FK_RESLINK_TARGET")
  .toColumn("TARGET_RESOURCE_ID").references("HFJ_RESOURCE", "RES_ID");
  }
+
+ {
+ version.onTable("MPI_LINK")
+ .addIndex("20230504.1", "IDX_EMPI_GR_TGT")
+ .unique(false)
+ .withColumns("GOLDEN_RESOURCE_PID", "TARGET_PID");
+ }
+
  }
 
  protected void init640() {

hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/mdm/batch2/clear/MdmLinkSlowDeletionSandboxIT.java

@@ -0,0 +1,143 @@
+package ca.uhn.fhir.mdm.batch2.clear;
+
+import ca.uhn.fhir.jpa.entity.MdmLink;
+import ca.uhn.fhir.jpa.model.dao.JpaPid;
+import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
+import ca.uhn.fhir.jpa.test.config.TestR4Config;
+import ca.uhn.fhir.mdm.api.MdmLinkSourceEnum;
+import ca.uhn.fhir.mdm.api.MdmMatchResultEnum;
+import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
+import ca.uhn.fhir.util.StopWatch;
+import org.apache.commons.dbcp2.BasicDataSource;
+import org.hibernate.dialect.PostgreSQL9Dialect;
+import org.hl7.fhir.r4.model.Coding;
+import org.hl7.fhir.r4.model.Patient;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+import org.postgresql.Driver;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.test.context.ContextConfiguration;
+
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+
+import static ca.uhn.fhir.mdm.api.MdmConstants.CODE_GOLDEN_RECORD;
+import static ca.uhn.fhir.mdm.api.MdmConstants.CODE_HAPI_MDM_MANAGED;
+import static ca.uhn.fhir.mdm.api.MdmConstants.SYSTEM_GOLDEN_RECORD_STATUS;
+import static ca.uhn.fhir.mdm.api.MdmConstants.SYSTEM_MDM_MANAGED;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@Disabled("Keeping as a sandbox to be used whenever we need a lot of MdmLinks in DB for performance testing")
+@ContextConfiguration(classes = {MdmLinkSlowDeletionSandboxIT.TestDataSource.class})
+public class MdmLinkSlowDeletionSandboxIT extends BaseJpaR4Test {
+ private static final Logger ourLog = LoggerFactory.getLogger(MdmLinkSlowDeletionSandboxIT.class);
+
+ private final int ourMdmLinksToCreate = 1_000_000;
+ private final int ourLogMdmLinksEach = 1_000;
+
+ @Override
+ public void afterPurgeDatabase() {
+ // keep the generated data!
+// super.afterPurgeDatabase();
+ }
+
+ @Disabled
+ @Test
+ void createMdmLinks() {
+ generatePatientsAndMdmLinks(ourMdmLinksToCreate);
+
+ long totalLinks = myMdmLinkDao.count();
+ ourLog.info("Total links in DB: {}", totalLinks);
+ assertTrue(totalLinks > 0);
+ }
+
+
+ private void generatePatientsAndMdmLinks(int theLinkCount) {
+ StopWatch sw = new StopWatch();
+ int totalMdmLinksCreated = 0;
+
+ for (int i = 0; i < theLinkCount; i++) {
+ List<JpaPid> patientIds = createMdmLinkPatients();
+
+ createMdmLink(patientIds.get(0), patientIds.get(1));
+ totalMdmLinksCreated++;
+
+ if (totalMdmLinksCreated % ourLogMdmLinksEach == 0) {
+ ourLog.info("Total MDM links created: {} in {} - ETA: {}", totalMdmLinksCreated, sw,
+ sw.getEstimatedTimeRemaining(totalMdmLinksCreated, ourMdmLinksToCreate));
+ }
+ }
+ }
+
+ private void createMdmLink(JpaPid thePidSource, JpaPid thePidTarget) {
+ MdmLink link = new MdmLink();
+ link.setGoldenResourcePersistenceId( thePidSource );
+ link.setSourcePersistenceId( thePidTarget );
+ Date now = new Date();
+ link.setCreated(now);
+ link.setUpdated(now);
+ link.setVersion("1");
+ link.setLinkSource(MdmLinkSourceEnum.MANUAL);
+ link.setMatchResult(MdmMatchResultEnum.MATCH);
+ link.setMdmSourceType("Patient");
+ link.setEidMatch(false);
+ link.setHadToCreateNewGoldenResource(true);
+ link.setRuleCount(6L);
+ link.setScore(.8);
+ link.setVector(61L);
+ runInTransaction(() -> myEntityManager.persist(link));
+ }
+
+ private List<JpaPid> createMdmLinkPatients() {
+ List<JpaPid> patientIds = new ArrayList<>();
+ for (int i = 0; i < 2; i++) {
+ Patient patient = new Patient();
+ patient.addName().setFamily(String.format("lastn-%07d", i)).addGiven(String.format("name-%07d", i));
+ if (i % 2 == 1) {
+ patient.getMeta()
+ .addTag(new Coding().setSystem(SYSTEM_MDM_MANAGED).setCode(CODE_HAPI_MDM_MANAGED));
+ } else {
+ patient.getMeta()
+ .addTag(new Coding().setSystem(SYSTEM_GOLDEN_RECORD_STATUS).setCode(CODE_GOLDEN_RECORD));
+ }
+ Long pId = myPatientDao.create(patient, new SystemRequestDetails()).getId().getIdPartAsLong();
+ JpaPid jpaPid = JpaPid.fromIdAndResourceType(pId, "Patient");
+ patientIds.add(jpaPid);
+ }
+ return patientIds;
+ }
+
+ @Configuration
+ public static class TestDataSource extends TestR4Config {
+
+ @Override
+ public String getHibernateDialect() {
+ return PostgreSQL9Dialect.class.getName();
+
+// return Oracle12cDialect.class.getName();
+ }
+
+ @Override
+ public void setConnectionProperties(BasicDataSource theDataSource) {
+ theDataSource.setDriver(new Driver());
+ theDataSource.setUrl("jdbc:postgresql:https://localhost/mdm_link_perf");
+ theDataSource.setMaxWaitMillis(30000);
+ theDataSource.setUsername("cdr");
+ theDataSource.setPassword("smileCDR");
+ theDataSource.setMaxTotal(ourMaxThreads);
+
+// theDataSource.setDriver(DriverTypeEnum.ORACLE_12C);
+// theDataSource.setUrl("jdbc:oracle:thin:@localhost:1527/cdr.localdomain");
+// theDataSource.setMaxWaitMillis(30000);
+// theDataSource.setUsername("cdr");
+// theDataSource.setPassword("smileCDR");
+// theDataSource.setMaxTotal(ourMaxThreads);
+ }
+ }
+
+}
+
+

hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/interceptor/MdmStorageInterceptor.java

@@ -49,6 +49,10 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor {
 
  private static final Logger ourLog = LoggerFactory.getLogger(MdmStorageInterceptor.class);
 
+ // Used to bypass trying to remove mdm links associated to a resource when running mdm-clear batch job, which
+ // deletes all links beforehand, and impacts performance for no action
+ private static final ThreadLocal<Boolean> ourLinksDeletedBeforehand = ThreadLocal.withInitial(() -> Boolean.FALSE);
+
  @Autowired
  private IExpungeEverythingService myExpungeEverythingService;
  @Autowired
@@ -124,10 +128,13 @@ public void blockManualGoldenResourceManipulationOnUpdate(IBaseResource theOldRe
 
  @Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_DELETED)
  public void deleteMdmLinks(RequestDetails theRequest, IBaseResource theResource) {
- if (!myMdmSettings.isSupportedMdmType(myFhirContext.getResourceType(theResource))) {
+ if (ourLinksDeletedBeforehand.get()) {
  return;
  }
- myMdmLinkDeleteSvc.deleteWithAnyReferenceTo(theResource);
+
+ if (myMdmSettings.isSupportedMdmType(myFhirContext.getResourceType(theResource))) {
+ myMdmLinkDeleteSvc.deleteWithAnyReferenceTo(theResource);
+ }
  }
 
  private void forbidIfModifyingExternalEidOnTarget(IBaseResource theNewResource, IBaseResource theOldResource) {
@@ -219,4 +226,13 @@ public void expungeAllMatchedMdmLinks(AtomicInteger theCounter, IBaseResource th
  ourLog.debug("Expunging MdmLink records with reference to {}", theResource.getIdElement());
  theCounter.addAndGet(myMdmLinkDeleteSvc.deleteWithAnyReferenceTo(theResource));
  }
+
+ public static void setLinksDeletedBeforehand() {
+ ourLinksDeletedBeforehand.set(Boolean.TRUE);
+ }
+
+ public static void resetLinksDeletedBeforehand() {
+ ourLinksDeletedBeforehand.remove();
+ }
+
 }

hapi-fhir-storage-mdm/src/main/java/ca/uhn/fhir/mdm/batch2/clear/MdmClearStep.java

@@ -30,14 +30,14 @@
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
 import ca.uhn.fhir.jpa.api.model.DeleteConflictList;
-import ca.uhn.fhir.jpa.api.model.DeleteMethodOutcome;
 import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
 import ca.uhn.fhir.jpa.dao.tx.HapiTransactionService;
 import ca.uhn.fhir.jpa.delete.DeleteConflictUtil;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
-import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.mdm.dao.IMdmLinkDao;
+import ca.uhn.fhir.mdm.interceptor.MdmStorageInterceptor;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
+import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
 import ca.uhn.fhir.rest.server.provider.ProviderConstants;
 import ca.uhn.fhir.util.StopWatch;
@@ -108,28 +108,42 @@ public Void doInTransaction(@Nonnull TransactionStatus theStatus) {
  return null;
  }
 
- ourLog.info("Starting mdm clear work chunk with {} resources - Instance[{}] Chunk[{}]", persistentIds.size(), myInstanceId, myChunkId);
+ // avoid double deletion of mdm links
+ MdmStorageInterceptor.setLinksDeletedBeforehand();
+
+ try {
+ performWork(persistentIds);
+
+ } finally {
+ MdmStorageInterceptor.resetLinksDeletedBeforehand();
+ }
+
+ return null;
+ }
+
+ private void performWork(List<JpaPid> thePersistentIds) {
+ ourLog.info("Starting mdm clear work chunk with {} resources - Instance[{}] Chunk[{}]", thePersistentIds.size(), myInstanceId, myChunkId);
  StopWatch sw = new StopWatch();
 
- myMdmLinkSvc.deleteLinksWithAnyReferenceToPids(persistentIds);
+ myMdmLinkSvc.deleteLinksWithAnyReferenceToPids(thePersistentIds);
+ ourLog.trace("Deleted {} mdm links in {}", thePersistentIds.size(), StopWatch.formatMillis(sw.getMillis()));
 
  // We know the list is not empty, and that all resource types are the same, so just use the first one
  String resourceName = myData.getResourceType(0);
- IFhirResourceDao dao = myDaoRegistry.getResourceDao(resourceName);
+ IFhirResourceDao<?> dao = myDaoRegistry.getResourceDao(resourceName);
 
  DeleteConflictList conflicts = new DeleteConflictList();
- dao.deletePidList(ProviderConstants.OPERATION_MDM_CLEAR, persistentIds, conflicts, myRequestDetails);
+ dao.deletePidList(ProviderConstants.OPERATION_MDM_CLEAR, thePersistentIds, conflicts, myRequestDetails);
  DeleteConflictUtil.validateDeleteConflictsEmptyOrThrowException(myFhirContext, conflicts);
+ ourLog.trace("Deleted {} golden resources in {}", thePersistentIds.size(), StopWatch.formatMillis(sw.getMillis()));
 
- dao.expunge(persistentIds, myRequestDetails);
+ dao.expunge(thePersistentIds, myRequestDetails);
 
- ourLog.info("Finished removing {} golden resources in {} - {}/sec - Instance[{}] Chunk[{}]", persistentIds.size(), sw, sw.formatThroughput(persistentIds.size(), TimeUnit.SECONDS), myInstanceId, myChunkId);
+ ourLog.info("Finished removing {} golden resources in {} - {}/sec - Instance[{}] Chunk[{}]", thePersistentIds.size(), sw, sw.formatThroughput(thePersistentIds.size(), TimeUnit.SECONDS), myInstanceId, myChunkId);
 
  if (ourClearCompletionCallbackForUnitTest != null) {
  ourClearCompletionCallbackForUnitTest.run();
  }
-
- return null;
  }
  }