Skip to content

Commit 0a07cae

Browse files
authored
fix(migration): preempt PDTS duplicates and recover invalid index in 1.12.9 (#28238)
* fix(migration): preempt PDTS duplicates and recover invalid index in 1.12.9 CREATE UNIQUE INDEX CONCURRENTLY aborts when it hits existing duplicate keys but leaves an invalid index behind. On migration retry, IF NOT EXISTS no-ops successfully and gets checksum-logged, after which ADD CONSTRAINT USING INDEX fails permanently with "index ... is not valid". Hit at a customer with two duplicate table.systemProfile rows on a 10M-row PDTS. Adds two idempotent statements before the existing constraint build: - DO block: drops the invalid index and clears its migration-log entry when indisvalid=false. No-op on fresh DBs and on already-migrated environments (where the index is valid and owned by the constraint). - DELETE: collapses duplicate rows via single hash aggregate on the 4-column key + targeted self-join. Reads only key columns (no json scan), only touches rows in actual duplicate groups, no-op on clean DBs. Efficient on multi-million-row PDTS tables. Existing CREATE INDEX / ALTER TABLE / ANALYZE statements byte-identical so checksum-matched skips for already-migrated environments still apply. * fix(migration): self-heal in one pass + schema-scoped invalid-index probe Addresses Copilot review on #28238: 1. One-pass self-heal. MigrationFile.parseSQLFiles filters already-logged statements at parse time (MigrationFile.java:83). Clearing the CREATE log entry from inside a DO block doesn't bring CREATE back into the current pass's execution list — it would only re-run on the next migration cycle, leaving the same-pass ALTER to fail again. Replace the "DROP + clear log" pattern with "DROP + rebuild inline" so a valid index exists before ALTER runs in the same pass. Inline rebuild uses non-concurrent CREATE UNIQUE INDEX, which takes a brief ACCESS EXCLUSIVE lock on the table. Acceptable because this path fires only when the environment is already in a degraded state. Normal-path customers go through the CONCURRENTLY build below. 2. Schema-scoped invalid-index probe. pg_class.relname is not schema-unique. Anchor the lookup via i.indrelid = 'profiler_data_time_series'::regclass and DROP by index OID (invalid_idx::regclass), so an invalid index with the same name in another schema cannot accidentally trigger this branch. Existing CREATE INDEX / ALTER TABLE / ANALYZE statements byte-identical to before this PR, so checksum-matched skips still apply for already-migrated environments. Test gap (Copilot's third comment) for the recovery scenario tracked as follow-up — existing migration tests in MigrationWorkflowReprocessingTest are mock-based; verifying recovery end-to-end needs Postgres integration infrastructure. * chore(migration): trim verbose comments in 1.12.9/postgres/schemaChanges.sql Statement bodies unchanged — checksums identical. Detailed mechanism write-ups live in the commit log / PR description; the file keeps just the load-bearing intent comment above each statement. * fix(migration): scope PDTS dedup to operation IS NOT NULL Addresses Copilot review on PR #28238 discussion r3264066840. Postgres UNIQUE treats NULLs as DISTINCT by default, so the constraint on (entityFQNHash, extension, operation, timestamp) permits multiple rows where operation IS NULL — i.e. table.tableProfile and table.columnProfile rows that share the other key columns. The previous dedup used GROUP BY which treats NULLs as equal, so it would have collapsed retry-induced tableProfile / columnProfile pairs that the restored constraint never actually blocked. Restricting the subquery to operation IS NOT NULL (plus a defensive entityFQNHash IS NOT NULL) aligns dedup with constraint semantics. DMG's customer rows were all table.systemProfile (operation = INSERT), so this still removes the customer dupes correctly. tableProfile / columnProfile retry duplicates — if they exist — stay as-is, which is the same outcome the unique constraint would produce on its own. * perf(migration): boost work_mem / maintenance_work_mem for PDTS dedup at scale Mirrors the tuning pattern from 1.9.9/postgres/postDataMigrationSQLScript.sql (same table, same operation class). On 50M-row PDTS the dedup DELETE's hash aggregate spills to disk with default work_mem=4MB, adding ~30-60s of disk I/O. Bumping work_mem to 256MB keeps the aggregate in memory; maintenance_work_mem=512MB lets CREATE UNIQUE INDEX CONCURRENTLY sort in memory too. Session-level (not SET LOCAL) because schemaChanges runs in autocommit (CREATE INDEX CONCURRENTLY requires it) — SET LOCAL would reset between statements. RESET at the end of the file restores defaults before the connection returns to the Hikari pool. Expected runtime impact at customer scale: 20M rows: ~30s tuned vs ~40s default 50M rows: ~40s tuned vs ~90s default (avoids spill) * chore(migration): trim comments on PDTS dedup additions * chore(migration): drop 1.9.9 reference from mem comment
1 parent fe4ad64 commit 0a07cae

1 file changed

Lines changed: 48 additions & 0 deletions

File tree

bootstrap/sql/migrations/native/1.12.9/postgres/schemaChanges.sql

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,47 @@
1+
-- Boost memory for the dedup + index build. RESET at end.
2+
SET work_mem = '256MB';
3+
SET maintenance_work_mem = '512MB';
4+
5+
-- Dedup before the unique index rebuild. NULL filter on operation: Postgres
6+
-- UNIQUE treats NULLs as DISTINCT, so the constraint never blocked tableProfile
7+
-- / columnProfile rows (operation = NULL). GROUP BY treats NULLs as equal —
8+
-- without the filter we'd collapse rows the constraint never rejected.
9+
DELETE FROM profiler_data_time_series p
10+
USING (
11+
SELECT entityFQNHash, extension, operation, "timestamp", MAX(ctid) AS keep_ctid
12+
FROM profiler_data_time_series
13+
WHERE operation IS NOT NULL
14+
AND entityFQNHash IS NOT NULL
15+
GROUP BY entityFQNHash, extension, operation, "timestamp"
16+
HAVING COUNT(*) > 1
17+
) d
18+
WHERE p.entityFQNHash = d.entityFQNHash
19+
AND p.extension = d.extension
20+
AND p.operation = d.operation
21+
AND p."timestamp" = d."timestamp"
22+
AND p.ctid <> d.keep_ctid;
23+
24+
-- Recover from a prior failed CREATE UNIQUE INDEX CONCURRENTLY: drop the
25+
-- invalid leftover and rebuild inline so ALTER below can promote it.
26+
DO $$
27+
DECLARE
28+
invalid_idx oid;
29+
BEGIN
30+
SELECT i.indexrelid INTO invalid_idx
31+
FROM pg_index i
32+
JOIN pg_class idx ON idx.oid = i.indexrelid
33+
WHERE idx.relname = 'profiler_data_time_series_unique_hash_extension_ts'
34+
AND i.indrelid = 'profiler_data_time_series'::regclass
35+
AND NOT i.indisvalid;
36+
37+
IF invalid_idx IS NOT NULL THEN
38+
EXECUTE 'DROP INDEX ' || invalid_idx::regclass;
39+
EXECUTE 'CREATE UNIQUE INDEX profiler_data_time_series_unique_hash_extension_ts '
40+
|| 'ON profiler_data_time_series '
41+
|| '(entityFQNHash, extension, operation, "timestamp")';
42+
END IF;
43+
END $$;
44+
145
-- Restore the unique constraint dropped in 1.9.9. Closes the 1.9.9 regression that caused
246
-- /columns?fields=profile 504s, and brings Postgres back in line with MySQL (which never
347
-- lost it). The leading (entityFQNHash, extension) prefix serves the column-profile batch query.
@@ -12,3 +56,7 @@ ALTER TABLE profiler_data_time_series
1256
UNIQUE USING INDEX profiler_data_time_series_unique_hash_extension_ts;
1357

1458
ANALYZE profiler_data_time_series;
59+
60+
-- Reset session memory before the connection returns to the pool.
61+
RESET work_mem;
62+
RESET maintenance_work_mem;

0 commit comments

Comments
 (0)