From a4ca7738ef9fb7ea4079e957bf4b76ecbe510a36 Mon Sep 17 00:00:00 2001 From: Christopher Schultz Date: Wed, 8 Mar 2023 11:12:12 -0500 Subject: [PATCH 1/5] Use a deep copy of query stats whose values won't change during sorting. Fixes https://bz.apache.org/bugzilla/show_bug.cgi?id=58489 --- .../pool/interceptor/SlowQueryReport.java | 47 ++++++++++++++++--- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java index 4198d3759e29..86a2a1a90576 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java @@ -222,19 +222,54 @@ protected QueryStats getQueryStats(String sql) { return qs; } + static class MiniQueryStats { + public final QueryStats queryStats; + public final long lastInvocation; + + public MiniQueryStats(QueryStats queryStats) { + this.queryStats = queryStats; + this.lastInvocation = queryStats.lastInvocation; + } + } + + static class MiniQueryStatsComparator implements Comparator + { + @Override + public int compare(MiniQueryStats stats1, MiniQueryStats stats2) { + return Long.compare(handleZero(stats1.lastInvocation), + handleZero(stats2.lastInvocation)); + } + + private static long handleZero(long value) { + return value == 0 ? Long.MAX_VALUE : value; + } + } + + private MiniQueryStatsComparator miniQueryStatsComparator = new MiniQueryStatsComparator(); + /** * Sort QueryStats by last invocation time * @param queries The queries map */ protected void removeOldest(ConcurrentHashMap queries) { - ArrayList list = new ArrayList<>(queries.values()); - Collections.sort(list, queryStatsComparator); + // Make a defensive deep-copy of the query stats list to prevent + // concurrent changes to the lastModified member during list-sort + ArrayList list = new ArrayList<>(queries.size()); + for(QueryStats stats : queries.values()) + list.add(new MiniQueryStats(stats)); + + Collections.sort(list, miniQueryStatsComparator); int removeIndex = 0; while (queries.size() > maxQueries) { - String sql = list.get(removeIndex).getQuery(); - queries.remove(sql); - if (log.isDebugEnabled()) { - log.debug("Removing slow query, capacity reached:"+sql); + MiniQueryStats mqs = list.get(removeIndex); + // Check to see if the lastInvocation has been updated since we + // took our snapshot. If the timestamps disagree, it means + // that this item is no longer the oldest (and it likely now + // one of the newest). + if(mqs.lastInvocation == mqs.queryStats.lastInvocation) { + String sql = mqs.queryStats.query; + queries.remove(sql); + if (log.isDebugEnabled()) log.debug("Removing slow query, capacity reached:"+sql); } removeIndex++; } From f237d05c2020662e31b22b9adf97dd3817685b25 Mon Sep 17 00:00:00 2001 From: Christopher Schultz Date: Thu, 9 Mar 2023 09:04:21 -0500 Subject: [PATCH 2/5] Match coding style. --- .../apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java index 86a2a1a90576..2410e07278a0 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java @@ -255,8 +255,9 @@ protected void removeOldest(ConcurrentHashMap queries) { // Make a defensive deep-copy of the query stats list to prevent // concurrent changes to the lastModified member during list-sort ArrayList list = new ArrayList<>(queries.size()); - for(QueryStats stats : queries.values()) + for(QueryStats stats : queries.values()) { list.add(new MiniQueryStats(stats)); + } Collections.sort(list, miniQueryStatsComparator); int removeIndex = 0; From d86e97032e82bb6ae4f70cc3368a2ee657e94f1a Mon Sep 17 00:00:00 2001 From: Christopher Schultz Date: Mon, 20 Oct 2025 12:26:39 -0400 Subject: [PATCH 3/5] Don't run off the end of the list --- .../jdbc/pool/interceptor/SlowQueryReport.java | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java index 2410e07278a0..b63b12a73be3 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java @@ -258,10 +258,18 @@ protected void removeOldest(ConcurrentHashMap queries) { for(QueryStats stats : queries.values()) { list.add(new MiniQueryStats(stats)); } + int queryCount = list.size(); Collections.sort(list, miniQueryStatsComparator); + int removeIndex = 0; - while (queries.size() > maxQueries) { + // Remove old queries until we have fewer than maxQueries or + // run out of queries to remove. If there is high enough turnover + // in the queries map, this one-time process may not remove enough + // old queries. We will rely on this process running multiple + // times to eventually reduce the query count back down to less + // than the maxQueries count. + while (queries.size() > maxQueries && removeIndex < queryCount) { MiniQueryStats mqs = list.get(removeIndex); // Check to see if the lastInvocation has been updated since we // took our snapshot. If the timestamps disagree, it means @@ -270,7 +278,9 @@ protected void removeOldest(ConcurrentHashMap queries) { if(mqs.lastInvocation == mqs.queryStats.lastInvocation) { String sql = mqs.queryStats.query; queries.remove(sql); - if (log.isDebugEnabled()) log.debug("Removing slow query, capacity reached:"+sql); + if (log.isDebugEnabled()) { + log.debug("Removing slow query, capacity reached:"+sql); + } } removeIndex++; } From 2ed85468979204c959882c37f7f3a3ee943ea256 Mon Sep 17 00:00:00 2001 From: Christopher Schultz Date: Mon, 20 Oct 2025 12:30:35 -0400 Subject: [PATCH 4/5] Add WARN if we weren't able to remove enough queries --- .../apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java index b63b12a73be3..ebb6762d6c5e 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java @@ -284,6 +284,9 @@ protected void removeOldest(ConcurrentHashMap queries) { } removeIndex++; } + if (queries.size() > maxQueries) { + log.warn("removeOldest: unable to remove enough old queries; will try again next time"); + } } From 73af7be84e53ae3f9f9be3144b1f141209071f2d Mon Sep 17 00:00:00 2001 From: Christopher Schultz Date: Mon, 20 Oct 2025 12:43:58 -0400 Subject: [PATCH 5/5] Make checkstyle happy --- .../apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java index 14a0561facd6..39d6887c80e9 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java @@ -220,7 +220,7 @@ static class MiniQueryStats { public final QueryStats queryStats; public final long lastInvocation; - public MiniQueryStats(QueryStats queryStats) { + MiniQueryStats(QueryStats queryStats) { this.queryStats = queryStats; this.lastInvocation = queryStats.lastInvocation; }