4324 excessive amount of time required to update large group resources (#4332)

* Initial test.

* Solution implementation with test adjustment.

* Adding changelog.

* Modifications following first code review.

* Moving changelog to 6.4.0

* Further reductions in query counts

* Fix for broken test

* Test fix

Co-authored-by: James Agnew <jamesagnew@gmail.com>
This commit is contained in:
Etienne Poirier 2022-12-07 10:27:33 -05:00 committed by GitHub
parent 3556e0fdf8
commit f13b247c74
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 213 additions and 22 deletions

View File

@ -0,0 +1,5 @@
---
type: fix
issue: 4324
jira: SMILE-5560
title: "Previously, an excessive amount of time was required to perform update on a large Group. This has been corrected."

View File

@ -21,6 +21,7 @@ import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
import ca.uhn.fhir.jpa.api.svc.ISearchCoordinatorSvc;
import ca.uhn.fhir.jpa.dao.data.IForcedIdDao;
import ca.uhn.fhir.jpa.dao.data.IResourceHistoryTableDao;
import ca.uhn.fhir.jpa.dao.data.IResourceLinkDao;
import ca.uhn.fhir.jpa.dao.data.IResourceTableDao;
import ca.uhn.fhir.jpa.dao.data.IResourceTagDao;
import ca.uhn.fhir.jpa.dao.expunge.ExpungeService;
@ -39,6 +40,7 @@ import ca.uhn.fhir.jpa.model.entity.PartitionablePartitionId;
import ca.uhn.fhir.jpa.model.entity.ResourceEncodingEnum;
import ca.uhn.fhir.jpa.model.entity.ResourceHistoryProvenanceEntity;
import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
import ca.uhn.fhir.jpa.model.entity.ResourceLink;
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
import ca.uhn.fhir.jpa.model.entity.ResourceTag;
import ca.uhn.fhir.jpa.model.entity.TagDefinition;
@ -55,6 +57,7 @@ import ca.uhn.fhir.jpa.sp.ISearchParamPresenceSvc;
import ca.uhn.fhir.jpa.term.api.ITermReadSvc;
import ca.uhn.fhir.jpa.util.AddRemoveCount;
import ca.uhn.fhir.jpa.util.MemoryCacheService;
import ca.uhn.fhir.jpa.util.QueryChunker;
import ca.uhn.fhir.model.api.IResource;
import ca.uhn.fhir.model.api.ResourceMetadataKeyEnum;
import ca.uhn.fhir.model.api.StorageResponseCodeEnum;
@ -214,6 +217,8 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
@Autowired
protected IResourceTableDao myResourceTableDao;
@Autowired
protected IResourceLinkDao myResourceLinkDao;
@Autowired
protected IResourceTagDao myResourceTagDao;
@Autowired
protected DeleteConflictService myDeleteConflictService;
@ -951,6 +956,22 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
existingParams = existingSearchParams.get(entity);
if (existingParams == null) {
existingParams = new ResourceIndexedSearchParams(entity);
/*
* If we have lots of resource links, this proactively fetches the targets so
* that we don't look them up one-by-one when comparing the new set to the
* old set later on
*/
if (existingParams.getResourceLinks().size() >= 10) {
List<Long> pids = existingParams
.getResourceLinks()
.stream()
.map(t->t.getId())
.collect(Collectors.toList());
new QueryChunker<Long>().chunk(pids, t->{
List<ResourceLink> targets = myResourceLinkDao.findByPidAndFetchTargetDetails(t);
ourLog.trace("Prefetched targets: {}", targets);
});
}
existingSearchParams.put(entity, existingParams);
}
entity.setDeleted(null);

View File

@ -39,4 +39,13 @@ public interface IResourceLinkDao extends JpaRepository<ResourceLink, Long>, IHa
@Query("SELECT t FROM ResourceLink t WHERE t.myTargetResourcePid in :resIds")
List<ResourceLink> findWithTargetPidIn(@Param("resIds") List<Long> thePids);
/**
* Loads a collection of ResourceLink entities by PID, but also eagerly fetches
* the target resources and their forced IDs
*/
@Query("SELECT t FROM ResourceLink t LEFT JOIN FETCH t.myTargetResource tr LEFT JOIN FETCH tr.myForcedId WHERE t.myId in :pids")
List<ResourceLink> findByPidAndFetchTargetDetails(@Param("pids") List<Long> thePids);
}

View File

@ -128,7 +128,7 @@ public class SearchParamWithInlineReferencesExtractor {
public void populateFromResource(RequestPartitionId theRequestPartitionId, ResourceIndexedSearchParams theParams, TransactionDetails theTransactionDetails, ResourceTable theEntity, IBaseResource theResource, ResourceIndexedSearchParams theExistingParams, RequestDetails theRequest, boolean theFailOnInvalidReference) {
extractInlineReferences(theResource, theTransactionDetails, theRequest);
mySearchParamExtractorService.extractFromResource(theRequestPartitionId, theRequest, theParams, theEntity, theResource, theTransactionDetails, theFailOnInvalidReference);
mySearchParamExtractorService.extractFromResource(theRequestPartitionId, theRequest, theParams, theExistingParams, theEntity, theResource, theTransactionDetails, theFailOnInvalidReference);
ResourceSearchParams activeSearchParams = mySearchParamRegistry.getActiveSearchParams(theEntity.getResourceType());
if (myDaoConfig.getIndexMissingFields() == DaoConfig.IndexEnabledEnum.ENABLED) {

View File

@ -143,7 +143,7 @@ public class ResourceLink extends BaseResourceIndex {
b.append(myTargetResourceUrl, obj.myTargetResourceUrl);
b.append(myTargetResourceType, obj.myTargetResourceType);
b.append(myTargetResourceVersion, obj.myTargetResourceVersion);
b.append(getTargetResourceId(), obj.getTargetResourceId());
b.append(getTargetResourcePid(), obj.getTargetResourcePid());
return b.isEquals();
}
@ -256,8 +256,7 @@ public class ResourceLink extends BaseResourceIndex {
b.append(mySourceResource);
b.append(myTargetResourceUrl);
b.append(myTargetResourceVersion);
b.append(getTargetResourceType());
b.append(getTargetResourceId());
b.append(getTargetResourcePid());
return b.toHashCode();
}

View File

@ -62,10 +62,13 @@ import org.hl7.fhir.instance.model.api.IIdType;
import org.springframework.beans.factory.annotation.Autowired;
import javax.annotation.Nonnull;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Date;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import static org.apache.commons.lang3.StringUtils.isBlank;
import static org.apache.commons.lang3.StringUtils.isNotBlank;
@ -92,34 +95,39 @@ public class SearchParamExtractorService {
mySearchParamExtractor = theSearchParamExtractor;
}
public void extractFromResource(RequestPartitionId theRequestPartitionId, RequestDetails theRequestDetails, ResourceIndexedSearchParams theParams, ResourceTable theEntity, IBaseResource theResource, TransactionDetails theTransactionDetails, boolean theFailOnInvalidReference) {
extractFromResource(theRequestPartitionId, theRequestDetails, theParams, new ResourceIndexedSearchParams(), theEntity, theResource, theTransactionDetails, theFailOnInvalidReference);
}
/**
* This method is responsible for scanning a resource for all of the search parameter instances. I.e. for all search parameters defined for
* a given resource type, it extracts the associated indexes and populates {@literal theParams}.
*/
public void extractFromResource(RequestPartitionId theRequestPartitionId, RequestDetails theRequestDetails, ResourceIndexedSearchParams theParams, ResourceTable theEntity, IBaseResource theResource, TransactionDetails theTransactionDetails, boolean theFailOnInvalidReference) {
public void extractFromResource(RequestPartitionId theRequestPartitionId, RequestDetails theRequestDetails, ResourceIndexedSearchParams theNewParams, ResourceIndexedSearchParams theExistingParams, ResourceTable theEntity, IBaseResource theResource, TransactionDetails theTransactionDetails, boolean theFailOnInvalidReference) {
// All search parameter types except Reference
ResourceIndexedSearchParams normalParams = new ResourceIndexedSearchParams();
extractSearchIndexParameters(theRequestDetails, normalParams, theResource);
mergeParams(normalParams, theParams);
mergeParams(normalParams, theNewParams);
if (myModelConfig.isIndexOnContainedResources()) {
ResourceIndexedSearchParams containedParams = new ResourceIndexedSearchParams();
extractSearchIndexParametersForContainedResources(theRequestDetails, containedParams, theResource, theEntity);
mergeParams(containedParams, theParams);
mergeParams(containedParams, theNewParams);
}
// Do this after, because we add to strings during both string and token processing, and contained resource if any
populateResourceTables(theParams, theEntity);
populateResourceTables(theNewParams, theEntity);
// Reference search parameters
extractResourceLinks(theRequestPartitionId, theParams, theEntity, theResource, theTransactionDetails, theFailOnInvalidReference, theRequestDetails);
extractResourceLinks(theRequestPartitionId, theExistingParams, theNewParams, theEntity, theResource, theTransactionDetails, theFailOnInvalidReference, theRequestDetails);
if (myModelConfig.isIndexOnContainedResources()) {
extractResourceLinksForContainedResources(theRequestPartitionId, theParams, theEntity, theResource, theTransactionDetails, theFailOnInvalidReference, theRequestDetails);
extractResourceLinksForContainedResources(theRequestPartitionId, theNewParams, theEntity, theResource, theTransactionDetails, theFailOnInvalidReference, theRequestDetails);
}
theParams.setUpdatedTime(theTransactionDetails.getTransactionDate());
theNewParams.setUpdatedTime(theTransactionDetails.getTransactionDate());
}
@VisibleForTesting
@ -289,6 +297,10 @@ public class SearchParamExtractorService {
}
private void extractResourceLinks(RequestPartitionId theRequestPartitionId, ResourceIndexedSearchParams theParams, ResourceTable theEntity, IBaseResource theResource, TransactionDetails theTransactionDetails, boolean theFailOnInvalidReference, RequestDetails theRequest) {
extractResourceLinks(theRequestPartitionId, new ResourceIndexedSearchParams(), theParams, theEntity, theResource, theTransactionDetails, theFailOnInvalidReference, theRequest);
}
private void extractResourceLinks(RequestPartitionId theRequestPartitionId, ResourceIndexedSearchParams theExistingParams, ResourceIndexedSearchParams theNewParams, ResourceTable theEntity, IBaseResource theResource, TransactionDetails theTransactionDetails, boolean theFailOnInvalidReference, RequestDetails theRequest) {
String sourceResourceName = myContext.getResourceType(theResource);
ISearchParamExtractor.SearchParamSet<PathAndRef> refs = mySearchParamExtractor.extractResourceLinks(theResource, false);
@ -296,13 +308,13 @@ public class SearchParamExtractorService {
for (PathAndRef nextPathAndRef : refs) {
RuntimeSearchParam searchParam = mySearchParamRegistry.getActiveSearchParam(sourceResourceName, nextPathAndRef.getSearchParamName());
extractResourceLinks(theRequestPartitionId, theParams, theEntity, theTransactionDetails, sourceResourceName, searchParam, nextPathAndRef, theFailOnInvalidReference, theRequest);
extractResourceLinks(theRequestPartitionId, theExistingParams, theNewParams, theEntity, theTransactionDetails, sourceResourceName, searchParam, nextPathAndRef, theFailOnInvalidReference, theRequest);
}
theEntity.setHasLinks(theParams.myLinks.size() > 0);
theEntity.setHasLinks(theNewParams.myLinks.size() > 0);
}
private void extractResourceLinks(@Nonnull RequestPartitionId theRequestPartitionId, ResourceIndexedSearchParams theParams, ResourceTable theEntity, TransactionDetails theTransactionDetails, String theSourceResourceName, RuntimeSearchParam theRuntimeSearchParam, PathAndRef thePathAndRef, boolean theFailOnInvalidReference, RequestDetails theRequest) {
private void extractResourceLinks(@Nonnull RequestPartitionId theRequestPartitionId, ResourceIndexedSearchParams theExistingParams, ResourceIndexedSearchParams theNewParams, ResourceTable theEntity, TransactionDetails theTransactionDetails, String theSourceResourceName, RuntimeSearchParam theRuntimeSearchParam, PathAndRef thePathAndRef, boolean theFailOnInvalidReference, RequestDetails theRequest) {
IBaseReference nextReference = thePathAndRef.getRef();
IIdType nextId = nextReference.getReferenceElement();
String path = thePathAndRef.getPath();
@ -321,13 +333,13 @@ public class SearchParamExtractorService {
nextId = nextId.toVersionless();
}
theParams.myPopulatedResourceLinkParameters.add(thePathAndRef.getSearchParamName());
theNewParams.myPopulatedResourceLinkParameters.add(thePathAndRef.getSearchParamName());
boolean canonical = thePathAndRef.isCanonical();
if (LogicalReferenceHelper.isLogicalReference(myModelConfig, nextId) || canonical) {
String value = nextId.getValue();
ResourceLink resourceLink = ResourceLink.forLogicalReference(thePathAndRef.getPath(), theEntity, value, transactionDate);
if (theParams.myLinks.add(resourceLink)) {
if (theNewParams.myLinks.add(resourceLink)) {
ourLog.debug("Indexing remote resource reference URL: {}", nextId);
}
return;
@ -369,7 +381,7 @@ public class SearchParamExtractorService {
throw new InvalidRequestException(Msg.code(507) + msg);
} else {
ResourceLink resourceLink = ResourceLink.forAbsoluteReference(thePathAndRef.getPath(), theEntity, nextId, transactionDate);
if (theParams.myLinks.add(resourceLink)) {
if (theNewParams.myLinks.add(resourceLink)) {
ourLog.debug("Indexing remote resource reference URL: {}", nextId);
}
return;
@ -410,7 +422,21 @@ public class SearchParamExtractorService {
* if the reference is invalid
*/
myResourceLinkResolver.validateTypeOrThrowException(type);
resourceLink = resolveTargetAndCreateResourceLinkOrReturnNull(theRequestPartitionId, theSourceResourceName, thePathAndRef, theEntity, transactionDate, nextId, theRequest, theTransactionDetails);
/**
* We need to obtain a resourceLink out of the provided {@literal thePathAndRef}. In the case
* where we are updating a resource that already has resourceLinks (stored in {@literal theExistingParams.getResourceLinks()}),
* let's try to match thePathAndRef to an already existing resourceLink to avoid the
* very expensive operation of creating a resourceLink that would end up being exactly the same
* one we already have.
*/
Optional<ResourceLink> optionalResourceLink = findMatchingResourceLink(thePathAndRef, theExistingParams.getResourceLinks());
if(optionalResourceLink.isPresent()){
resourceLink = optionalResourceLink.get();
} else {
resourceLink = resolveTargetAndCreateResourceLinkOrReturnNull(theRequestPartitionId, theSourceResourceName, thePathAndRef, theEntity, transactionDate, nextId, theRequest, theTransactionDetails);
}
if (resourceLink == null) {
return;
} else {
@ -432,7 +458,30 @@ public class SearchParamExtractorService {
}
theParams.myLinks.add(resourceLink);
theNewParams.myLinks.add(resourceLink);
}
private Optional<ResourceLink> findMatchingResourceLink(PathAndRef thePathAndRef, Collection<ResourceLink> theResourceLinks) {
IIdType referenceElement = thePathAndRef.getRef().getReferenceElement();
List<ResourceLink> resourceLinks = new ArrayList<>(theResourceLinks);
for (ResourceLink resourceLink : resourceLinks) {
// comparing the searchParam path ex: Group.member.entity
boolean hasMatchingSearchParamPath = StringUtils.equals(resourceLink.getSourcePath(), thePathAndRef.getPath());
boolean hasMatchingResourceType = StringUtils.equals(resourceLink.getTargetResourceType(), referenceElement.getResourceType());
boolean hasMatchingResourceId = StringUtils.equals(resourceLink.getTargetResourceId(), referenceElement.getIdPart());
boolean hasMatchingResourceVersion = myContext.getParserOptions().isStripVersionsFromReferences() || referenceElement.getVersionIdPartAsLong() == null || referenceElement.getVersionIdPartAsLong().equals(resourceLink.getTargetResourceVersion());
if( hasMatchingSearchParamPath && hasMatchingResourceType && hasMatchingResourceId && hasMatchingResourceVersion) {
return Optional.of(resourceLink);
}
}
return Optional.empty();
}
private void extractResourceLinksForContainedResources(RequestPartitionId theRequestPartitionId, ResourceIndexedSearchParams theParams, ResourceTable theEntity, IBaseResource theResource, TransactionDetails theTransactionDetails, boolean theFailOnInvalidReference, RequestDetails theRequest) {

View File

@ -38,6 +38,7 @@ import org.hl7.fhir.r4.model.Coverage;
import org.hl7.fhir.r4.model.DateTimeType;
import org.hl7.fhir.r4.model.Encounter;
import org.hl7.fhir.r4.model.ExplanationOfBenefit;
import org.hl7.fhir.r4.model.Group;
import org.hl7.fhir.r4.model.IdType;
import org.hl7.fhir.r4.model.Location;
import org.hl7.fhir.r4.model.Narrative;
@ -59,6 +60,8 @@ import org.springframework.data.domain.Slice;
import javax.annotation.Nonnull;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;
@ -159,6 +162,50 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
assertEquals(0, myCaptureQueriesListener.getDeleteQueriesForCurrentThread().size());
}
@Test
public void testUpdateGroup_withAddedReferences_willSucceed(){
int initialPatientsCount = 30;
int newPatientsCount = 5;
int allPatientsCount = initialPatientsCount + newPatientsCount;
List<IIdType> patientList = createPatients(allPatientsCount);
myCaptureQueriesListener.clear();
Group group = createGroup(patientList.subList(0, initialPatientsCount));
assertQueryCount(31,0,4,0);
myCaptureQueriesListener.clear();
group = updateGroup(group, patientList.subList(initialPatientsCount, allPatientsCount));
assertQueryCount(10,1,2,0);
assertEquals(allPatientsCount, group.getMember().size());
}
@Test
public void testUpdateGroup_NoChangesToReferences(){
List<IIdType> patientList = createPatients(30);
myCaptureQueriesListener.clear();
Group group = createGroup(patientList);
assertQueryCount(31,0,4,0);
// Make a change to the group, but don't touch any references in it
myCaptureQueriesListener.clear();
group.addIdentifier().setValue("foo");
group = updateGroup(group, Collections.emptyList());
myCaptureQueriesListener.logSelectQueries();
assertQueryCount(5,1,2,0);
assertEquals(30, group.getMember().size());
}
@Test
public void testUpdateWithChangesAndTags() {
@ -2760,5 +2807,66 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
}
private void printQueryCount(String theMessage){
ourLog.info("QueryCount {} is: ", theMessage);
ourLog.info("\tselect: {}", myCaptureQueriesListener.getSelectQueriesForCurrentThread().size());
ourLog.info("\tupdate: {}", myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size());
ourLog.info("\tinsert: {}", myCaptureQueriesListener.getInsertQueriesForCurrentThread().size());
ourLog.info("\tdelete: {}", myCaptureQueriesListener.getDeleteQueriesForCurrentThread().size());
}
private void assertQueryCount(int theExpectedSelect, int theExpectedUpdate, int theExpectedInsert, int theExpectedDelete){
myCaptureQueriesListener.logSelectQueriesForCurrentThread();
assertEquals(theExpectedSelect, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size());
myCaptureQueriesListener.logUpdateQueriesForCurrentThread();
assertEquals(theExpectedUpdate, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size());
myCaptureQueriesListener.logInsertQueriesForCurrentThread();
assertEquals(theExpectedInsert, myCaptureQueriesListener.getInsertQueriesForCurrentThread().size());
myCaptureQueriesListener.logDeleteQueriesForCurrentThread();
assertEquals(theExpectedDelete, myCaptureQueriesListener.getDeleteQueriesForCurrentThread().size());
}
private Group createGroup(List<IIdType> theIIdTypeList) {
Group aGroup = new Group();
aGroup.setId("Group/someGroupId");
return updateGroup(aGroup, theIIdTypeList);
}
private Group updateGroup(Group theGroup, List<IIdType> theIIdTypeList) {
for (IIdType idType : theIIdTypeList) {
Group.GroupMemberComponent aGroupMemberComponent = new Group.GroupMemberComponent(new Reference(idType));
theGroup.addMember(aGroupMemberComponent);
}
Group retVal = runInTransaction(() -> (Group) myGroupDao.update(theGroup, mySrd).getResource());
return retVal;
}
private List<IIdType> createPatients(int theCount) {
List<IIdType> reVal = new ArrayList<>(theCount);
for (int i = 0; i < theCount; i++) {
reVal.add(createAPatient());
}
return reVal;
}
private IIdType createAPatient(){
IIdType retVal = runInTransaction(() -> {
Patient p = new Patient();
p.getMeta().addTag("http://system", "foo", "display");
p.addIdentifier().setSystem("urn:system").setValue("2");
return myPatientDao.create(p).getId().toUnqualified();
});
return retVal;
}
}

View File

@ -296,7 +296,7 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test {
myObservationDao.update(observation);
// Make sure we're not introducing any extra DB operations
assertEquals(6, myCaptureQueriesListener.logSelectQueries().size());
assertEquals(5, myCaptureQueriesListener.logSelectQueries().size());
// Read back and verify that reference is now versioned
observation = myObservationDao.read(observationId);

View File

@ -192,7 +192,7 @@ public class ReindexStepTest extends BaseJpaR4Test {
// Verify
assertEquals(2, outcome.getRecordsProcessed());
assertEquals(10, myCaptureQueriesListener.logSelectQueries().size());
assertEquals(8, myCaptureQueriesListener.logSelectQueries().size());
assertEquals(0, myCaptureQueriesListener.countInsertQueries());
assertEquals(4, myCaptureQueriesListener.countUpdateQueries());
assertEquals(0, myCaptureQueriesListener.countDeleteQueries());
@ -237,7 +237,7 @@ public class ReindexStepTest extends BaseJpaR4Test {
// Verify
assertEquals(4, outcome.getRecordsProcessed());
assertEquals(6, myCaptureQueriesListener.logSelectQueries().size());
assertEquals(5, myCaptureQueriesListener.logSelectQueries().size());
assertEquals(5, myCaptureQueriesListener.countInsertQueries());
assertEquals(2, myCaptureQueriesListener.countUpdateQueries());
assertEquals(0, myCaptureQueriesListener.countDeleteQueries());