From 5f91567ed17f96716e7a227702eed2ea96bb9e63 Mon Sep 17 00:00:00 2001 From: danielhanrahan Date: Mon, 20 Feb 2023 14:01:45 +0000 Subject: [PATCH] Improve performance of deleteDataNodes SQL - Use SQL IN operator instead of temp table when deleting nodes - Use Postgresql LIKE ANY array operator when deleting lists - Update delete perf test timings - Refactor adding cascade delete constraint Issue-ID: CPS-1502 Signed-off-by: danielhanrahan Change-Id: Ic90b867e7c71ec1981f05a9122322ece84dd8bde --- .../repository/FragmentNativeRepositoryImpl.java | 73 +++++++++++----------- .../CpsDataPersistenceServiceDeletePerfTest.groovy | 16 ++--- 2 files changed, 46 insertions(+), 43 deletions(-) diff --git a/cps-ri/src/main/java/org/onap/cps/spi/repository/FragmentNativeRepositoryImpl.java b/cps-ri/src/main/java/org/onap/cps/spi/repository/FragmentNativeRepositoryImpl.java index 0e4d359da..5c5458a03 100644 --- a/cps-ri/src/main/java/org/onap/cps/spi/repository/FragmentNativeRepositoryImpl.java +++ b/cps-ri/src/main/java/org/onap/cps/spi/repository/FragmentNativeRepositoryImpl.java @@ -21,8 +21,11 @@ package org.onap.cps.spi.repository; import java.util.Collection; +import java.util.Collections; +import java.util.stream.Collectors; import javax.persistence.EntityManager; import javax.persistence.PersistenceContext; +import javax.persistence.Query; import lombok.RequiredArgsConstructor; @RequiredArgsConstructor @@ -40,55 +43,55 @@ public class FragmentNativeRepositoryImpl implements FragmentNativeRepository { @PersistenceContext private final EntityManager entityManager; - private final TempTableCreator tempTableCreator; - @Override public void deleteFragmentEntity(final long fragmentEntityId) { entityManager.createNativeQuery( - DROP_FRAGMENT_CONSTRAINT - + ADD_FRAGMENT_CONSTRAINT_WITH_CASCADE - + "DELETE FROM fragment WHERE id = ?;" - + DROP_FRAGMENT_CONSTRAINT - + ADD_ORIGINAL_FRAGMENT_CONSTRAINT) + addFragmentConstraintWithDeleteCascade("DELETE FROM fragment WHERE id = ?")) .setParameter(1, fragmentEntityId) .executeUpdate(); } @Override - // Accept security hotspot as temporary table name in SQL query is created internally, not from user input. - @SuppressWarnings("squid:S2077") public void deleteByAnchorIdAndXpaths(final int anchorId, final Collection xpaths) { - if (!xpaths.isEmpty()) { - final String tempTableName = tempTableCreator.createTemporaryTable("xpathsToDelete", xpaths, "xpath"); - entityManager.createNativeQuery( - DROP_FRAGMENT_CONSTRAINT - + ADD_FRAGMENT_CONSTRAINT_WITH_CASCADE - + "DELETE FROM fragment f USING " + tempTableName + " t" - + " WHERE f.anchor_id = :anchorId AND f.xpath = t.xpath;" - + DROP_FRAGMENT_CONSTRAINT - + ADD_ORIGINAL_FRAGMENT_CONSTRAINT) - .setParameter("anchorId", anchorId) - .executeUpdate(); - } + final String queryString = addFragmentConstraintWithDeleteCascade( + "DELETE FROM fragment f WHERE f.anchor_id = ? AND (f.xpath IN (:parameterPlaceholders))"); + executeUpdateWithAnchorIdAndCollection(queryString, anchorId, xpaths); } @Override - // Accept security hotspot as temporary table name in SQL query is created internally, not from user input. + public void deleteListsByAnchorIdAndXpaths(final int anchorId, final Collection listXpaths) { + final Collection listXpathPatterns = + listXpaths.stream().map(listXpath -> listXpath + "[%").collect(Collectors.toSet()); + final String queryString = addFragmentConstraintWithDeleteCascade( + "DELETE FROM fragment f WHERE f.anchor_id = ? AND (f.xpath LIKE ANY (array[:parameterPlaceholders]))"); + executeUpdateWithAnchorIdAndCollection(queryString, anchorId, listXpathPatterns); + } + + // Accept security hotspot as placeholders in SQL query are created internally, not from user input. @SuppressWarnings("squid:S2077") - public void deleteListsByAnchorIdAndXpaths(final int anchorId, final Collection xpaths) { - if (!xpaths.isEmpty()) { - final String tempTableName = tempTableCreator.createTemporaryTable("xpathsToDelete", xpaths, "xpath"); - entityManager.createNativeQuery( - DROP_FRAGMENT_CONSTRAINT - + ADD_FRAGMENT_CONSTRAINT_WITH_CASCADE - + "DELETE FROM fragment f USING " + tempTableName + " t" - + " WHERE f.anchor_id = :anchorId AND f.xpath LIKE CONCAT(t.xpath, :xpathListPattern);" - + DROP_FRAGMENT_CONSTRAINT - + ADD_ORIGINAL_FRAGMENT_CONSTRAINT) - .setParameter("anchorId", anchorId) - .setParameter("xpathListPattern", "[%%") - .executeUpdate(); + private void executeUpdateWithAnchorIdAndCollection(final String sqlTemplate, final int anchorId, + final Collection collection) { + if (!collection.isEmpty()) { + final String parameterPlaceholders = String.join(",", Collections.nCopies(collection.size(), "?")); + final String queryStringWithParameterPlaceholders = + sqlTemplate.replaceFirst(":parameterPlaceholders\\b", parameterPlaceholders); + + final Query query = entityManager.createNativeQuery(queryStringWithParameterPlaceholders); + query.setParameter(1, anchorId); + int parameterIndex = 2; + for (final String parameterValue : collection) { + query.setParameter(parameterIndex++, parameterValue); + } + query.executeUpdate(); } } + private static String addFragmentConstraintWithDeleteCascade(final String queryString) { + return DROP_FRAGMENT_CONSTRAINT + + ADD_FRAGMENT_CONSTRAINT_WITH_CASCADE + + queryString + ";" + + DROP_FRAGMENT_CONSTRAINT + + ADD_ORIGINAL_FRAGMENT_CONSTRAINT; + } + } diff --git a/cps-ri/src/test/groovy/org/onap/cps/spi/performance/CpsDataPersistenceServiceDeletePerfTest.groovy b/cps-ri/src/test/groovy/org/onap/cps/spi/performance/CpsDataPersistenceServiceDeletePerfTest.groovy index db09382ab..8e74b6222 100644 --- a/cps-ri/src/test/groovy/org/onap/cps/spi/performance/CpsDataPersistenceServiceDeletePerfTest.groovy +++ b/cps-ri/src/test/groovy/org/onap/cps/spi/performance/CpsDataPersistenceServiceDeletePerfTest.groovy @@ -67,8 +67,8 @@ class CpsDataPersistenceServiceDeletePerfTest extends CpsPersistencePerfSpecBase objectUnderTest.deleteDataNodes(PERF_DATASPACE, PERF_ANCHOR, xpathsToDelete) stopWatch.stop() def deleteDurationInMillis = stopWatch.getTotalTimeMillis() - then: 'delete duration is under 300 milliseconds' - recordAndAssertPerformance('Batch delete 100 children', 300, deleteDurationInMillis) + then: 'delete duration is under 250 milliseconds' + recordAndAssertPerformance('Batch delete 100 children', 250, deleteDurationInMillis) } def 'Delete 50 grandchildren (that have no descendants)'() { @@ -97,8 +97,8 @@ class CpsDataPersistenceServiceDeletePerfTest extends CpsPersistencePerfSpecBase objectUnderTest.deleteDataNodes(PERF_DATASPACE, PERF_ANCHOR, xpathsToDelete) stopWatch.stop() def deleteDurationInMillis = stopWatch.getTotalTimeMillis() - then: 'delete duration is under 300 milliseconds' - recordAndAssertPerformance('Batch delete 500 grandchildren', 300, deleteDurationInMillis) + then: 'delete duration is under 125 milliseconds' + recordAndAssertPerformance('Batch delete 500 grandchildren', 125, deleteDurationInMillis) } @Sql([CLEAR_DATA, PERF_TEST_DATA]) @@ -135,8 +135,8 @@ class CpsDataPersistenceServiceDeletePerfTest extends CpsPersistencePerfSpecBase objectUnderTest.deleteDataNodes(PERF_DATASPACE, PERF_ANCHOR, xpathsToDelete) stopWatch.stop() def deleteDurationInMillis = stopWatch.getTotalTimeMillis() - then: 'delete duration is under 350 milliseconds' - recordAndAssertPerformance('Batch delete 100 whole lists', 350, deleteDurationInMillis) + then: 'delete duration is under 250 milliseconds' + recordAndAssertPerformance('Batch delete 100 whole lists', 250, deleteDurationInMillis) } def 'Delete 10 list elements'() { @@ -165,8 +165,8 @@ class CpsDataPersistenceServiceDeletePerfTest extends CpsPersistencePerfSpecBase objectUnderTest.deleteDataNodes(PERF_DATASPACE, PERF_ANCHOR, xpathsToDelete) stopWatch.stop() def deleteDurationInMillis = stopWatch.getTotalTimeMillis() - then: 'delete duration is under 300 milliseconds' - recordAndAssertPerformance('Batch delete 500 lists elements', 300, deleteDurationInMillis) + then: 'delete duration is under 125 milliseconds' + recordAndAssertPerformance('Batch delete 500 lists elements', 125, deleteDurationInMillis) } @Sql([CLEAR_DATA, PERF_TEST_DATA]) -- 2.16.6