Skip to content

Commit 4cf9014

Browse files
notSumit25claude
notSumit25
and
claude
committed
perf/fix: close the remaining Performance-tab gaps from the readiness review
The seven non-blocking findings left over after #86. Each was re-verified against current main before being fixed, and each fix verified after — two of them turned out to be wrong on the first attempt, both caught by testing rather than reading. Sample recovery was an unindexable full scan on a table nothing pruned recoverFullText runs one lookup per sample, up to 20 per "view full query" click, and the query wrapped query_text in three nested regexp_replace/REPLACE calls plus LOWER — so no index could satisfy it and Postgres materialized a rewritten copy of the whole per-connection slice. EXPLAIN (ANALYZE) on a real install: 36 ms at 1,093 rows, 1,112 ms at 34,976. Linear, and query_lineage was absent from SlowQueryRetentionService, so it degraded with the install's *age* rather than its load — which is why it passed every pre-launch test. Precomputed into a STORED generated column (normalized_match) with a text_pattern_ops index, applied by QueryLineageMatchIndexInitializer since this repo has no Flyway runtime. Verified on the real table: Index Scan, 0.119 ms, and the DDL is idempotent on re-run. query_lineage is now purged with the other three fact tables. Deliberately no COALESCE fallback to the inline expression. That is the obvious way to stay safe on a database without the column and it silently undoes the entire fix: measured on the same 37,504-row table, COALESCE(normalized_match, …) plans a Seq Scan at 48.7 ms versus 0.39 ms for the bare column. A missing column instead surfaces as a WARN from the initializer, and recoverFullText already catches a failed lookup and returns the sample unchanged. Denials were retried three times queryClient set retry: 3 with no status predicate, so a 403 became four requests and ~7 s of backoff before the UI could render anything — and an unauthorized /tenant-column-suggestions opens a fresh JDBC connection to the target database on every attempt. The first version of this fix read error.response.status and did nothing at all: the axios response interceptor rethrows a plain Error with the status copied onto error.status, so the axios-shaped field never matched. Measured in the browser before and after: 4 attempts / 7197 ms -> 1 attempt / 34 ms. Confirmed 500, 503, 401 and network failures still retry, and the 3-attempt cap still holds. A customer id containing a slash was unreachable by any encoding customerId is a literal value from the tenant column — application data, so it can contain /, ? or #. Raw, the slash split the path; percent-encoded, Jetty answers 400 "Ambiguous URI path separator". Both reproduced with the real value `acct/77?x=1`, whose rows rendered as "no queries rolled up yet" while the header said the customer had 12 executions. encodeURIComponent alone does not fix this, which is why the id moved off the path: /{connectionId}/customer-queries?customerId=… and /customer-query-samples. The old path routes are kept and @deprecated for wire compatibility. Verified: the slash-bearing id now returns 200, and 403 on a connection the caller cannot read. Failed fetches rendered as "no data yet" Every panel branched on `!isLoading && rows.length === 0`, and `data ?? []` turns any error into an empty array — so a 404 and an empty result were indistinguishable. That matters more now that connection authorization is enforced: a 403 would read as "nothing captured yet" and send the user to re-run an ingestion they cannot fix. Added a shared QueryError component wired into CustomerExplorer (3 branches), QueryTrendsTab (2) and WorkloadAnalysisPanel (1), with per-status wording verified against the real interceptor error shapes. Tab bar and ARIA At 390 px the four tabs measured 427 px with overflow-x: visible, so three of them sat off-screen unreachable. The bar now scrolls (verified: scrolls 373 px, all four reachable). Completed the ARIA tabs pattern — role="tabpanel", aria-controls, roving tabindex and arrow/Home/End navigation. The first version had a stale-closure bug that moved selection exactly once and then froze; fixed with the functional state updater, verified across the full key sequence including wraparound. Workload reads were gated on the write tier status/latest/getReport/history all used assertCanManageConnectionContent. EffectiveConnectionAccess's own comment lists slow-query analytics under read. Latent today because every grant resolves to FULL_CONTENT, but it would deny the whole Workload tab to a read-only grant the moment one is reintroduced. `run` keeps manage. Known issue, documented rather than fixed The slow-log ingestion cursor has two real defects, both left in place with a comment at updateLastProcessed explaining them: it records the time ingestion *finished* rather than the last event's timestamp (so events arriving mid-run are skipped permanently), and it writes LocalDateTime.now() while every read does .atZone(ZoneOffset.UTC) (agreeing only because the container runs Etc/UTC; a bare-metal install at UTC+5:30 would skip 5.5 h of history every run). Not fixed here because all six providers need live cloud credentials to exercise, and an unverified change to a cursor silently skips or duplicates data. Verification Backend and frontend both build clean. Live against the rebuilt image: new customer routes 200 with a slash-bearing id and 403 on an ungranted connection; workload reads non-403 for a granted user; the lineage index confirmed as an Index Scan on the real table. Not covered: mvn test was not run locally (no JDK/Maven on this host) — CI runs it. Note that the "backend tests (advisory)" job is continue-on-error, so its green tick means the job finished, not that tests passed; main currently has 5 failing test classes independent of this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 64a0085 commit 4cf9014

16 files changed

Lines changed: 461 additions & 43 deletions

‎backend/src/main/java/com/dbaagent/config/QueryLineageMatchIndexInitializer.java‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
package com.dbaagent.config;
2+
3+
import lombok.extern.slf4j.Slf4j;
4+
import org.springframework.context.annotation.Bean;
5+
import org.springframework.context.annotation.Configuration;
6+
import org.springframework.context.annotation.DependsOn;
7+
import org.springframework.jdbc.core.JdbcTemplate;
8+
9+
import javax.sql.DataSource;
10+
11+
/**
12+
* Adds the stored normalized column and index that make slow-query sample recovery
13+
* indexable.
14+
*
15+
* <p>{@code QueryLineageRepository.findLongestByConnectionIdAndNormalizedQueryTextPrefix}
16+
* wraps {@code query_text} in three nested {@code regexp_replace}/{@code REPLACE} calls
17+
* plus {@code LOWER} before comparing it, so no index on {@code query_text} can ever
18+
* satisfy the predicate — Postgres must materialize a rewritten copy of every row in the
19+
* connection's slice. {@code EXPLAIN (ANALYZE)} on a real install:
20+
*
21+
* <pre>
22+
* Seq Scan on query_lineage (actual time=0.244..35.826 rows=422 loops=1)
23+
* Execution Time: 36.179 ms -- at 1,093 rows
24+
* Execution Time: 1111.996 ms -- same query, table scaled to 34,976 rows
25+
* </pre>
26+
*
27+
* <p>{@code SlowQueryAnalyticsService.recoverFullText} runs that once per sample, up to
28+
* 20 per "view full query" click, so one modal open costs ~0.7 s today and ~22 s at 35k
29+
* rows. It degrades with <em>age</em> rather than load, which is why it passes every
30+
* pre-launch test: {@code query_lineage} is not pruned by
31+
* {@code SlowQueryRetentionService} (that only touches {@code slow_query_run},
32+
* {@code slow_query_customer_day} and {@code slow_query_sample}), so it only grows.
33+
*
34+
* <p>Precomputing the normalization into a STORED generated column pays the regex chain
35+
* once at write time. Measured on the same scaled table, with the ~120-character prefix
36+
* the caller actually sends:
37+
*
38+
* <pre>
39+
* Index Scan using idx_query_lineage_norm_match
40+
* Execution Time: 0.428 ms -- vs 1111.996 ms
41+
* </pre>
42+
*
43+
* <p>The index earns its keep only because the prefix is long and therefore selective. A
44+
* short prefix such as {@code 'select%'} still plans as a sequential scan (~50 ms at 35k
45+
* rows) — that is the precomputation alone, and is fine. Do not "simplify" this by
46+
* dropping the generated column and indexing {@code query_text} directly; the expression,
47+
* not the column, is what the query compares.
48+
*
49+
* <p>There is no Flyway runtime in this repo (see CLAUDE.md), so this initializer is what
50+
* actually applies {@code V118__add_query_lineage_norm_match.sql}. Both statements are
51+
* {@code IF NOT EXISTS} and the whole thing is best-effort: a failure here costs
52+
* performance, never correctness, since the query returns identical rows either way.
53+
*/
54+
@Configuration
55+
@Slf4j
56+
public class QueryLineageMatchIndexInitializer {
57+
58+
private static final String TABLE = "query_lineage";
59+
private static final String COLUMN = "normalized_match";
60+
private static final String INDEX = "idx_query_lineage_norm_match";
61+
62+
/**
63+
* Must match {@code SlowQueryAnalyticsService.normalizeForMatching} exactly, and the
64+
* expression already inlined in the repository query. If one changes, all three do.
65+
*/
66+
private static final String NORMALIZE_EXPR =
67+
"lower(regexp_replace(regexp_replace("
68+
+ "replace(query_text, '`', ''), "
69+
+ "'\\s*([.,();])\\s*', '\\1', 'g'), "
70+
+ "'\\s+', ' ', 'g'))";
71+
72+
@Bean("queryLineageMatchIndexBootstrap")
73+
@DependsOn("entityManagerFactory")
74+
public Object queryLineageMatchIndexBootstrap(DataSource dataSource) {
75+
JdbcTemplate jdbc = new JdbcTemplate(dataSource);
76+
77+
if (!tableExists(jdbc, TABLE)) {
78+
return new Object();
79+
}
80+
81+
try {
82+
jdbc.execute("ALTER TABLE " + TABLE + " ADD COLUMN IF NOT EXISTS " + COLUMN
83+
+ " text GENERATED ALWAYS AS (" + NORMALIZE_EXPR + ") STORED");
84+
} catch (RuntimeException e) {
85+
// Generated columns need Postgres 12+. Older servers keep the sequential scan,
86+
// which is slow but correct, so this must not stop the application.
87+
log.warn("Could not add {}.{} ({}); sample recovery stays on a sequential scan",
88+
TABLE, COLUMN, e.getMessage());
89+
return new Object();
90+
}
91+
92+
try {
93+
// text_pattern_ops so a LIKE 'prefix%' comparison can use the index under any
94+
// collation; the default opclass only helps in the C collation.
95+
jdbc.execute("CREATE INDEX IF NOT EXISTS " + INDEX + " ON " + TABLE
96+
+ " (connection_id, " + COLUMN + " text_pattern_ops)");
97+
} catch (RuntimeException e) {
98+
log.warn("Could not create {}: {}", INDEX, e.getMessage());
99+
}
100+
101+
return new Object();
102+
}
103+
104+
private static boolean tableExists(JdbcTemplate jdbc, String table) {
105+
try {
106+
Integer found = jdbc.queryForObject(
107+
"SELECT COUNT(*) FROM information_schema.tables "
108+
+ "WHERE table_schema = current_schema() AND table_name = ?",
109+
Integer.class, table);
110+
return found != null && found > 0;
111+
} catch (RuntimeException e) {
112+
log.warn("Could not check for table {}: {}", table, e.getMessage());
113+
return false;
114+
}
115+
}
116+
}

‎backend/src/main/java/com/dbaagent/controller/SlowQueryAnalyticsController.java‎

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,41 @@ public ResponseEntity<List<SlowQueryAnalyticsService.CustomerSummary>> listCusto
7878
return ResponseEntity.ok(analyticsService.listCustomers(connectionId));
7979
}
8080

81-
/** Every slow query attributed to one customer, ranked by their mean exec time. */
81+
/**
82+
* Every slow query attributed to one customer, ranked by their mean exec time.
83+
*
84+
* <p>The customer id is a <b>query parameter</b>, not a path segment, because it is a
85+
* literal value read out of the tenant column — application data, which can contain
86+
* {@code /}, {@code ?} or {@code #}. Such an id is unreachable as a path segment under
87+
* any encoding: raw, the slash splits the path; percent-encoded, Jetty rejects it with
88+
* {@code 400 Ambiguous URI path separator}. Both were reproduced against this backend
89+
* with the tenant value {@code acct/77?x=1}, whose rows were simply invisible in the
90+
* By-Customer view.
91+
*/
92+
@GetMapping("/{connectionId}/customer-queries")
93+
public ResponseEntity<List<SlowQueryAnalyticsService.CustomerQueryRow>> customerQueries(
94+
@PathVariable String connectionId,
95+
@RequestParam String customerId) {
96+
accessControlService.assertCanReadConnectionContent(connectionId);
97+
return ResponseEntity.ok(analyticsService.queriesForCustomer(connectionId, customerId));
98+
}
99+
100+
/** Literal-bearing samples for one (customer, query) pair — copyable SQL. */
101+
@GetMapping("/{connectionId}/customer-query-samples")
102+
public ResponseEntity<List<SlowQueryAnalyticsService.QuerySample>> customerQuerySamples(
103+
@PathVariable String connectionId,
104+
@RequestParam String customerId,
105+
@RequestParam String fingerprint) {
106+
accessControlService.assertCanReadConnectionContent(connectionId);
107+
return ResponseEntity.ok(
108+
analyticsService.samplesForCustomerQuery(connectionId, customerId, fingerprint));
109+
}
110+
111+
/**
112+
* @deprecated superseded by {@link #customerQueries}; a customer id containing a
113+
* slash cannot be expressed here. Retained so existing clients keep working.
114+
*/
115+
@Deprecated
82116
@GetMapping("/{connectionId}/customer/{customerId}/queries")
83117
public ResponseEntity<List<SlowQueryAnalyticsService.CustomerQueryRow>> queriesForCustomer(
84118
@PathVariable String connectionId,
@@ -87,7 +121,10 @@ public ResponseEntity<List<SlowQueryAnalyticsService.CustomerQueryRow>> queriesF
87121
return ResponseEntity.ok(analyticsService.queriesForCustomer(connectionId, customerId));
88122
}
89123

90-
/** Literal-bearing samples for one (customer, query) pair — copyable SQL. */
124+
/**
125+
* @deprecated superseded by {@link #customerQuerySamples}; see above.
126+
*/
127+
@Deprecated
91128
@GetMapping("/{connectionId}/customer/{customerId}/query/{fingerprint}/samples")
92129
public ResponseEntity<List<SlowQueryAnalyticsService.QuerySample>> samplesForCustomerQuery(
93130
@PathVariable String connectionId,

‎backend/src/main/java/com/dbaagent/controller/WorkloadAnalysisController.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@
2727
@Slf4j
2828
public class WorkloadAnalysisController {
2929

30+
// Reads use canReadContent; only `run` requires canManageContent. The reads were all
31+
// gated on manage, which is the write tier — EffectiveConnectionAccess's own comment
32+
// lists slow-query analytics under read. Latent today because every grant resolves to
33+
// FULL_CONTENT (so both predicates are true), but it would deny the whole Workload tab
34+
// to a read-only grant the moment one is reintroduced.
35+
3036
private final WorkloadAnalysisService workloadAnalysisService;
3137
private final WorkloadAnalysisReportRepository reportRepository;
3238
private final AccessControlService accessControlService;
@@ -64,7 +70,7 @@ public ResponseEntity<Map<String, Object>> run(@PathVariable String connectionId
6470
/** Lightweight poll payload — status + progress without the full report blob. */
6571
@GetMapping("/{connectionId}/status")
6672
public ResponseEntity<Map<String, Object>> status(@PathVariable String connectionId) {
67-
accessControlService.assertCanManageConnectionContent(connectionId);
73+
accessControlService.assertCanReadConnectionContent(connectionId);
6874
return reportRepository.findFirstByConnectionIdOrderByStartedAtDesc(connectionId)
6975
.map(r -> ResponseEntity.ok(Map.<String, Object>of(
7076
"reportId", r.getId(),
@@ -81,7 +87,7 @@ public ResponseEntity<Map<String, Object>> status(@PathVariable String connectio
8187
/** The full latest report (with the composed sections). 204 if none yet. */
8288
@GetMapping("/{connectionId}/latest")
8389
public ResponseEntity<WorkloadAnalysisReport> latest(@PathVariable String connectionId) {
84-
accessControlService.assertCanManageConnectionContent(connectionId);
90+
accessControlService.assertCanReadConnectionContent(connectionId);
8591
return reportRepository.findFirstByConnectionIdOrderByStartedAtDesc(connectionId)
8692
.map(ResponseEntity::ok)
8793
.orElseGet(() -> ResponseEntity.noContent().build());
@@ -92,7 +98,7 @@ public ResponseEntity<WorkloadAnalysisReport> latest(@PathVariable String connec
9298
public ResponseEntity<WorkloadAnalysisReport> getReport(@PathVariable String reportId) {
9399
return reportRepository.findById(reportId)
94100
.map(r -> {
95-
accessControlService.assertCanManageConnectionContent(r.getConnectionId());
101+
accessControlService.assertCanReadConnectionContent(r.getConnectionId());
96102
return ResponseEntity.ok(r);
97103
})
98104
.orElseGet(() -> ResponseEntity.notFound().build());
@@ -101,7 +107,7 @@ public ResponseEntity<WorkloadAnalysisReport> getReport(@PathVariable String rep
101107
/** Recent report history (newest first), metadata only via the entity. */
102108
@GetMapping("/{connectionId}/history")
103109
public ResponseEntity<List<WorkloadAnalysisReport>> history(@PathVariable String connectionId) {
104-
accessControlService.assertCanManageConnectionContent(connectionId);
110+
accessControlService.assertCanReadConnectionContent(connectionId);
105111
return ResponseEntity.ok(reportRepository.findTop20ByConnectionIdOrderByStartedAtDesc(connectionId));
106112
}
107113
}

‎backend/src/main/java/com/dbaagent/repository/QueryLineageRepository.java‎

Lines changed: 43 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@
33
import com.dbaagent.model.QueryLineage;
44
import org.springframework.data.jpa.repository.JpaRepository;
55
import org.springframework.data.jpa.repository.Query;
6+
import org.springframework.data.jpa.repository.Modifying;
67
import org.springframework.data.domain.Pageable;
78
import org.springframework.data.repository.query.Param;
9+
import org.springframework.transaction.annotation.Transactional;
810
import org.springframework.stereotype.Repository;
911

1012
import java.time.LocalDateTime;
@@ -147,23 +149,32 @@ QueryLineage findLongestByConnectionIdAndCollapsedQueryTextPrefix(
147149
* Callers must apply the SAME transformation to their prefix on the
148150
* Java side (see {@code SlowQueryAnalyticsService.normalizeForMatching}).
149151
*
150-
* <p>The regex chain runs once per candidate row, so this is intended for
151-
* single-row lookups (LIMIT 1) over the per-connection slice. For
152-
* d840f866-style connections (~5K lineage rows) it returns in a few ms;
153-
* larger connections may benefit from a functional index on the
154-
* normalized expression, but none is needed yet.
152+
* <p>The normalization is <b>precomputed</b> into the stored generated column
153+
* {@code normalized_match} ({@code QueryLineageMatchIndexInitializer}) and matched
154+
* against that, so the regex chain runs once at write time rather than once per
155+
* candidate row on every read. Inlining the expression here made the predicate
156+
* unindexable: Postgres had to materialize a rewritten copy of the whole
157+
* per-connection slice, which measured 36 ms at 1,093 rows and 1,112 ms at 34,976 —
158+
* and {@code recoverFullText} issues this up to 20 times per "view full query" click,
159+
* against a table no retention job prunes. On the same scaled table with the ~120-char
160+
* prefix the caller actually sends, the indexed form plans as an Index Scan at
161+
* 0.428 ms.
162+
*
163+
* <p>Matched against the column <b>directly</b>, with no {@code COALESCE} fallback to
164+
* the inline expression. That fallback is the obvious way to stay safe on a database
165+
* without the column, and it silently undoes the whole fix: wrapping the column in
166+
* {@code COALESCE(...)} makes the predicate non-indexable again. Measured on the same
167+
* 37,504-row table — {@code COALESCE(normalized_match, …)} plans a Seq Scan at
168+
* 48.7 ms, the bare column an Index Scan at 0.39 ms. If the column is ever absent,
169+
* {@code QueryLineageMatchIndexInitializer} logs it at WARN and
170+
* {@code SlowQueryAnalyticsService.recoverFullText} already treats a failed lookup as
171+
* "no longer text available" and returns the sample unchanged.
155172
*/
156173
@Query(value = """
157174
SELECT * FROM query_lineage
158175
WHERE connection_id = :connectionId
159176
AND LENGTH(query_text) > :minLength
160-
AND LOWER(
161-
regexp_replace(
162-
regexp_replace(
163-
REPLACE(query_text, '`', ''),
164-
'\\s*([.,();])\\s*', '\\1', 'g'),
165-
'\\s+', ' ', 'g')
166-
) LIKE :escapedPrefix ESCAPE '\\'
177+
AND normalized_match LIKE :escapedPrefix ESCAPE '\\'
167178
ORDER BY LENGTH(query_text) DESC
168179
LIMIT 1
169180
""", nativeQuery = true)
@@ -172,4 +183,24 @@ QueryLineage findLongestByConnectionIdAndNormalizedQueryTextPrefix(
172183
@Param("escapedPrefix") String escapedPrefix,
173184
@Param("minLength") int minLength
174185
);
186+
187+
/**
188+
* Drop lineage rows older than the connection's retention window.
189+
*
190+
* <p>This table was never pruned: {@code SlowQueryRetentionService} covered
191+
* {@code slow_query_run}, {@code slow_query_customer_day} and
192+
* {@code slow_query_sample} but not lineage, so it grew without bound while the
193+
* 30-day analytics tables stayed small. That is what made sample recovery degrade
194+
* with age rather than load — the scanned table kept growing even on an idle install.
195+
*
196+
* <p>Keyed on {@code created_at}, which is non-null and already indexed
197+
* ({@code idx_query_lineage_created}).
198+
*/
199+
@Modifying
200+
@Transactional
201+
@Query("DELETE FROM QueryLineage q WHERE q.connectionId = :connectionId "
202+
+ "AND q.createdAt < :cutoff")
203+
int deleteByConnectionIdAndCreatedAtBefore(
204+
@Param("connectionId") String connectionId,
205+
@Param("cutoff") java.time.LocalDateTime cutoff);
175206
}

‎backend/src/main/java/com/dbaagent/service/SlowLogIngestionService.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,30 @@ private boolean isFrequencySatisfied(SlowLogSourceConfig config) {
418418
return last.plusMinutes(freq).isBefore(LocalDateTime.now());
419419
}
420420

421+
/**
422+
* KNOWN ISSUE — this cursor has two defects, both unfixed. Deliberately left alone
423+
* rather than changed blind: every provider needs live cloud credentials to exercise,
424+
* so a fix cannot be verified end to end here, and getting it wrong silently skips or
425+
* duplicates slow-query events.
426+
*
427+
* <ol>
428+
* <li><b>Events arriving mid-run are lost.</b> The cursor is set to the time
429+
* ingestion <em>finished</em>, but {@code SINCE_LAST} then uses it as an
430+
* exclusive lower bound (see {@code resolveStartTime}). Anything whose timestamp
431+
* falls between the last parsed event and this write is never fetched — a gap
432+
* proportional to how long the run took, so worst on the slowest S3/CloudWatch
433+
* pulls. The fix is to record the maximum event timestamp actually parsed.
434+
* <li><b>Local time written, UTC read.</b> {@code LocalDateTime.now()} is
435+
* server-local; every read does {@code .atZone(ZoneOffset.UTC)}. The Compose
436+
* image runs {@code Etc/UTC} so the two agree there by luck, but on a bare-metal
437+
* install in, say, UTC+5:30, the cursor lands 5.5 h in the future and that much
438+
* slow-query history is skipped on every run (west of UTC it goes backwards and
439+
* re-ingests duplicates). The fix is to store an {@code Instant}/
440+
* {@code timestamptz} end to end.
441+
* </ol>
442+
*
443+
* <p>{@code SlowLogSourceConfigService.updateAfterAutoIngestion} has the same bug.
444+
*/
421445
private void updateLastProcessed(SlowLogSourceConfig config) {
422446
config.setLastProcessedAt(LocalDateTime.now());
423447
config.setUpdatedAt(LocalDateTime.now());

0 commit comments

Comments
 (0)