From da11d3d89ed655516e57e520fa09578491707942 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Sat, 15 Feb 2020 12:45:59 -0500 Subject: [PATCH] remove distinct from search (#1712) * it works now * oops * add successful migration message * add successful migration message * Add fix for #1300 * Remove FIXME * Test fix * Test fix Co-authored-by: James Agnew --- .../ca/uhn/fhir/jpa/dao/IResultIterator.java | 2 + .../ca/uhn/fhir/jpa/dao/SearchBuilder.java | 53 +++++++++++++++---- .../jpa/search/SearchCoordinatorSvcImpl.java | 3 +- .../search/SearchCoordinatorSvcImplTest.java | 39 ++++++-------- .../uhn/fhir/jpa/migrate/FlywayMigrator.java | 2 + .../fhir/jpa/migrate/TaskOnlyMigrator.java | 8 ++- 6 files changed, 74 insertions(+), 33 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IResultIterator.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IResultIterator.java index ce4795e9630..1b84f581849 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IResultIterator.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IResultIterator.java @@ -29,4 +29,6 @@ public interface IResultIterator extends Iterator, Closeab int getSkippedCount(); + int getNonSkippedCount(); + } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java index 07739e125ed..99b349c698d 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java @@ -254,7 +254,8 @@ public class SearchBuilder implements ISearchBuilder { outerQuery.multiselect(myBuilder.countDistinct(myQueryRoot.getRoot())); } else { outerQuery.multiselect(myQueryRoot.get("myId").as(Long.class)); - outerQuery.distinct(true); + // KHS This distinct call is causing performance issues in large installations +// outerQuery.distinct(true); } } @@ -965,6 +966,7 @@ public class SearchBuilder implements ISearchBuilder { private SortSpec mySort; private boolean myStillNeedToFetchIncludes; private int mySkipCount = 0; + private int myNonSkipCount = 0; private QueryIterator(SearchRuntimeDetails theSearchRuntimeDetails, RequestDetails theRequest) { mySearchRuntimeDetails = theSearchRuntimeDetails; @@ -994,14 +996,7 @@ public class SearchBuilder implements ISearchBuilder { myMaxResultsToFetch = myDaoConfig.getFetchSizeDefaultMaximum(); } - final TypedQuery query = createQuery(mySort, myMaxResultsToFetch, false, myRequest); - - mySearchRuntimeDetails.setQueryStopwatch(new StopWatch()); - - Query hibernateQuery = (Query) query; - hibernateQuery.setFetchSize(myFetchSize); - ScrollableResults scroll = hibernateQuery.scroll(ScrollMode.FORWARD_ONLY); - myResultsIterator = new ScrollableResultsIterator<>(scroll); + initializeIteratorQuery(myMaxResultsToFetch); // If the query resulted in extra results being requested if (myAlsoIncludePids != null) { @@ -1036,11 +1031,32 @@ public class SearchBuilder implements ISearchBuilder { ResourcePersistentId next = new ResourcePersistentId(nextLong); if (myPidSet.add(next)) { myNext = next; + myNonSkipCount++; break; } else { mySkipCount++; } } + + if (!myResultsIterator.hasNext()) { + if (myMaxResultsToFetch != null && (mySkipCount + myNonSkipCount == myMaxResultsToFetch)) { + if (mySkipCount > 0 && myNonSkipCount == 0) { + myMaxResultsToFetch += 1000; + + StorageProcessingMessage message = new StorageProcessingMessage(); + String msg = "Pass completed with no matching results. This indicates an inefficient query! Retrying with new max count of " + myMaxResultsToFetch; + ourLog.warn(msg); + message.setMessage(msg); + HookParams params = new HookParams() + .add(RequestDetails.class, myRequest) + .addIfMatchesType(ServletRequestDetails.class, myRequest) + .add(StorageProcessingMessage.class, message); + JpaInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, myRequest, Pointcut.JPA_PERFTRACE_WARNING, params); + + initializeIteratorQuery(myMaxResultsToFetch); + } + } + } } } @@ -1100,6 +1116,20 @@ public class SearchBuilder implements ISearchBuilder { } + private void initializeIteratorQuery(Integer theMaxResultsToFetch) { + final TypedQuery query = createQuery(mySort, theMaxResultsToFetch, false, myRequest); + + mySearchRuntimeDetails.setQueryStopwatch(new StopWatch()); + + Query hibernateQuery = (Query) query; + hibernateQuery.setFetchSize(myFetchSize); + ScrollableResults scroll = hibernateQuery.scroll(ScrollMode.FORWARD_ONLY); + myResultsIterator = new ScrollableResultsIterator<>(scroll); + + mySkipCount = 0; + myNonSkipCount = 0; + } + @Override public boolean hasNext() { if (myNext == null) { @@ -1122,6 +1152,11 @@ public class SearchBuilder implements ISearchBuilder { return mySkipCount; } + @Override + public int getNonSkippedCount() { + return myNonSkipCount; + } + @Override public void close() { if (myResultsIterator != null) { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index 731f4019148..49ef70e13b6 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -774,10 +774,11 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { if (theResultIter.hasNext() == false) { int skippedCount = theResultIter.getSkippedCount(); + int nonSkippedCount = theResultIter.getNonSkippedCount(); int totalFetched = skippedCount + myCountSavedThisPass + myCountBlockedThisPass; ourLog.trace("MaxToFetch[{}] SkippedCount[{}] CountSavedThisPass[{}] CountSavedThisTotal[{}] AdditionalPrefetchRemaining[{}]", myMaxResultsToFetch, skippedCount, myCountSavedThisPass, myCountSavedTotal, myAdditionalPrefetchThresholdsRemaining); - if (myMaxResultsToFetch != null && totalFetched < myMaxResultsToFetch) { + if (nonSkippedCount == 0 || (myMaxResultsToFetch != null && totalFetched < myMaxResultsToFetch)) { ourLog.trace("Setting search status to FINISHED"); mySearch.setStatus(SearchStatusEnum.FINISHED); mySearch.setTotalCount(myCountSavedTotal); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java index c3efb07cdc8..a622a7a4933 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java @@ -182,33 +182,11 @@ public class SearchCoordinatorSvcImplTest { when(mySearchBuilder.createQuery(any(), any(), any())).thenReturn(iter); doAnswer(loadPids()).when(mySearchBuilder).loadResourcesByPid(any(Collection.class), any(Collection.class), any(List.class), anyBoolean(), any()); - when(mySearchResultCacheSvc.fetchResultPids(any(), anyInt(), anyInt())).thenAnswer(t -> { - List returnedValues = iter.getReturnedValues(); - int offset = t.getArgument(1, Integer.class); - int end = t.getArgument(2, Integer.class); - end = Math.min(end, returnedValues.size()); - offset = Math.min(offset, returnedValues.size()); - ourLog.info("findWithSearchUuid {} - {} out of {} values", offset, end, returnedValues.size()); - return returnedValues.subList(offset, end); - }); - - when(mySearchResultCacheSvc.fetchAllResultPids(any())).thenReturn(allResults); - - when(mySearchCacheSvc.tryToMarkSearchAsInProgress(any())).thenAnswer(t->{ - Search search = t.getArgument(0, Search.class); - assertEquals(SearchStatusEnum.PASSCMPLET, search.getStatus()); - search.setStatus(SearchStatusEnum.LOADING); - return Optional.of(search); - }); - when(mySearchCacheSvc.save(any())).thenAnswer(t -> { Search search = t.getArgument(0, Search.class); myCurrentSearch = search; return search; }); - when(mySearchCacheSvc.fetchByUuid(any())).thenAnswer(t -> Optional.ofNullable(myCurrentSearch)); - IFhirResourceDao dao = myCallingDao; - when(myDaoRegistry.getResourceDao(any(String.class))).thenReturn(dao); IBundleProvider result = mySvc.registerSearch(myCallingDao, params, "Patient", new CacheControlDirective(), null); assertNotNull(result.getUuid()); @@ -602,6 +580,11 @@ public class SearchCoordinatorSvcImplTest { return myWrap.getSkippedCount(); } + @Override + public int getNonSkippedCount() { + return myCount; + } + @Override public void close() { // nothing @@ -611,6 +594,7 @@ public class SearchCoordinatorSvcImplTest { public static class ResultIterator extends BaseIterator implements IResultIterator { private final Iterator myWrap; + private int myCount; ResultIterator(Iterator theWrap) { myWrap = theWrap; @@ -623,6 +607,7 @@ public class SearchCoordinatorSvcImplTest { @Override public ResourcePersistentId next() { + myCount++; return myWrap.next(); } @@ -631,6 +616,11 @@ public class SearchCoordinatorSvcImplTest { return 0; } + @Override + public int getNonSkippedCount() { + return myCount; + } + @Override public void close() { // nothing @@ -697,6 +687,11 @@ public class SearchCoordinatorSvcImplTest { } } + @Override + public int getNonSkippedCount() { + return 0; + } + @Override public void close() { // nothing diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/FlywayMigrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/FlywayMigrator.java index a743ad281e7..8b60927345e 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/FlywayMigrator.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/FlywayMigrator.java @@ -63,6 +63,8 @@ public class FlywayMigrator extends BaseMigrator { if (isDryRun()) { StringBuilder statementBuilder = buildExecutedStatementsString(); ourLog.info("SQL that would be executed:\n\n***********************************\n{}***********************************", statementBuilder); + } else { + ourLog.info("Schema migrated successfully."); } } catch (Exception e) { throw e; diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/TaskOnlyMigrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/TaskOnlyMigrator.java index e27668fd0fe..86d1d92bcd6 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/TaskOnlyMigrator.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/TaskOnlyMigrator.java @@ -51,7 +51,11 @@ public class TaskOnlyMigrator extends BaseMigrator { next.setConnectionProperties(connectionProperties); try { - ourLog.info("Executing task of type: {}", next.getClass().getSimpleName()); + if (isDryRun()) { + ourLog.info("Dry run {} {}", next.getFlywayVersion(), next.getDescription()); + } else { + ourLog.info("Executing {} {}", next.getFlywayVersion(), next.getDescription()); + } next.execute(); addExecutedStatements(next.getExecutedStatements()); } catch (SQLException e) { @@ -61,6 +65,8 @@ public class TaskOnlyMigrator extends BaseMigrator { if (isDryRun()) { StringBuilder statementBuilder = buildExecutedStatementsString(); ourLog.info("SQL that would be executed:\n\n***********************************\n{}***********************************", statementBuilder); + } else { + ourLog.info("Schema migrated successfully."); } }