fix(tradein-sql): remove duplicate CREATE INDEX from 050 (regression from PR #500) #515

Merged

lekss361 merged 1 commit from fix/tradein-050-remove-dup-index-creates into main

2026-05-24 12:16:28 +00:00

lekss361 commented

2026-05-24 12:13:15 +00:00

Owner

Summary — production regression confirmed

PR #500 added 035_drop_duplicate_indexes.sql to drop listing_sources_listing_idx2 and houses_geom_idx2. Migration ran. But on 2026-05-24 ~12:08 production audit via docker exec tradein-postgres psql, both dupes are STILL present:

indexname
------------------------------
 houses_geom_idx
 houses_geom_idx2          ← should have been dropped
 listing_sources_listing_idx
 listing_sources_listing_idx2  ← should have been dropped

Root cause

Deploy runs for f in $(ls *.sql | sort); do psql < $f; done on every push (idempotent ops). Sort order in tradein-mvp/backend/data/sql/:

033_listings_alter_yandex.sql
034_trade_in_estimates_geom.sql
035_drop_duplicate_indexes.sql   ← drops idx2 indexes
040_houses_extend.sql
050_search_optimization.sql       ← re-CREATEs them (lines 80-82 + 89-91)

End state of every deploy: dupes restored.

Fix

Remove the offending CREATE INDEX IF NOT EXISTS statements from migration 050 at the source. Replace them with breadcrumb comments pointing to the canonical indexes in earlier migrations (009_houses.sql and 028_matching_tables.sql).

Migration 035 retained as the durable DROP guard — on next deploy it drops the existing dupes, and 050 no longer recreates them. Steady-state then matches PR #500's original intent.

Diff stats

1 file changed
CREATE INDEX count: 17 → 15 (delta -2, exactly the two removed)
File still has 2× BEGIN/COMMIT (indexes block + matview block, both intact)
No other DDL touched

Test plan

After deploy: docker exec tradein-postgres psql -U tradein -d tradein -c "\di+ listing_sources_*" shows _listing_idx only (no _idx2).
Same for \di+ houses_geom_* — houses_geom_idx only.
Other 15 indexes from 050 still present (listings_geom_active_idx, listings_active_filter_idx, listings_search_mv_* etc).

## Summary — production regression confirmed PR #500 added `035_drop_duplicate_indexes.sql` to drop `listing_sources_listing_idx2` and `houses_geom_idx2`. Migration ran. But on 2026-05-24 ~12:08 production audit via `docker exec tradein-postgres psql`, both dupes are STILL present: ``` indexname ------------------------------ houses_geom_idx houses_geom_idx2 ← should have been dropped listing_sources_listing_idx listing_sources_listing_idx2 ← should have been dropped ``` ## Root cause Deploy runs `for f in $(ls *.sql | sort); do psql < $f; done` on **every** push (idempotent ops). Sort order in `tradein-mvp/backend/data/sql/`: ``` 033_listings_alter_yandex.sql 034_trade_in_estimates_geom.sql 035_drop_duplicate_indexes.sql ← drops idx2 indexes 040_houses_extend.sql 050_search_optimization.sql ← re-CREATEs them (lines 80-82 + 89-91) ``` End state of every deploy: dupes restored. ## Fix Remove the offending `CREATE INDEX IF NOT EXISTS` statements from migration 050 at the source. Replace them with breadcrumb comments pointing to the canonical indexes in earlier migrations (`009_houses.sql` and `028_matching_tables.sql`). Migration 035 retained as the durable DROP guard — on next deploy it drops the existing dupes, and 050 no longer recreates them. Steady-state then matches PR #500's original intent. ## Diff stats - 1 file changed - CREATE INDEX count: 17 → 15 (delta -2, exactly the two removed) - File still has 2× BEGIN/COMMIT (indexes block + matview block, both intact) - No other DDL touched ## Test plan - [ ] After deploy: `docker exec tradein-postgres psql -U tradein -d tradein -c "\di+ listing_sources_*"` shows `_listing_idx` only (no `_idx2`). - [ ] Same for `\di+ houses_geom_*` — `houses_geom_idx` only. - [ ] Other 15 indexes from 050 still present (`listings_geom_active_idx`, `listings_active_filter_idx`, `listings_search_mv_*` etc).

lekss361 added 1 commit 2026-05-24 12:13:16 +00:00

fix(tradein-sql): remove duplicate CREATE INDEX from 050 (regression from PR #500 ) a10c47230b

PR #500 added 035_drop_duplicate_indexes.sql to drop listing_sources_listing_idx2
and houses_geom_idx2, but 050_search_optimization.sql re-CREATED them on every
deploy (sort order: 035 DROP → 050 CREATE). End state: dupes restored.

Verified on prod 2026-05-24 ~12:08 via docker exec tradein-postgres psql.

Fix: remove the two CREATE INDEX statements from 050 at the source. After this
PR, deploy order becomes: 035 DROPs (succeeds on existing dupes) → 050 doesn't
recreate → final state matches PR #500's intent.

Migration 035 retained as the durable DROP guard.

lekss361 commented

2026-05-24 12:16:21 +00:00

Author

Owner

Deep Code Review — verdict: APPROVE

Summary

Status: APPROVE
Files: 1 (P0: 1 — SQL migration)
Lines: +4 / -6
Scope: removes 2 redundant CREATE INDEX IF NOT EXISTS from 050_search_optimization.sql to close the PR #500 regression loop

Claim verification (all confirmed)

listing_sources_listing_idx is canonical in 028_matching_tables.sql:71 (btree on listing_id) — single-column, exact duplicate of removed _idx2.
houses_geom_idx is canonical in 009_houses.sql:58 (GIST on geom, non-partial) — strictly redundant with removed _idx2.
035_drop_duplicate_indexes.sql drops both _idx2 variants via DROP INDEX IF EXISTS (idempotent).
Migration sort order 028 → 035 → 050 confirmed via repo listing — alphanumeric sort puts 035 strictly before 050. Steady state after this PR: 028 creates canonical → 035 drops dupes (no-op on subsequent runs) → 050 no longer recreates them. Production dupes vanish on next deploy.

Idempotency / transactional safety

Both BEGIN; ... COMMIT; blocks (indexes + matview) intact.
All remaining CREATE INDEX IF NOT EXISTS preserved → file re-run safe.
deploy-tradein.yml runs for f in $(ls *.sql | sort); do psql -v ON_ERROR_STOP=on < $f; done — sort + ON_ERROR_STOP both honored. No deploy script changes needed.

Regression check

Repo-wide grep listing_sources_listing_idx2|houses_geom_idx2 → only 050 (being removed) and 035 (dropping). Zero code references — no query, service, or test depends on the _idx2 names.
tradein-mvp/scripts/probe-migration-028.py:21 checks for canonical listing_sources_listing_idx only — unaffected.
035 retained as durable DROP guard — correct strategy, since older deploys may still have dupes in prod that 050 had been re-creating until now.
The other _idx2-suffixed indexes in 050 (listing_sources_price_divergence_idx2, houses_fingerprint_idx2, houses_kadastr_idx2) are NOT removed and NOT dropped by 035 — confirmed they're unique (no canonical equivalent in earlier migrations). Suffix is just legacy naming, not duplicate. Correct call to leave them alone.

Architecture / archaeology

Replacing the removed CREATE INDEX lines with breadcrumb comments pointing to 028_matching_tables.sql and 009_houses.sql is the right call — future readers won't be confused why 050 "skips" indexes 8 and 10. Good hygiene.
Removal happens at the source (050), not by adding another DROP — minimal blast radius, no migration churn.

Tiny nit (non-blocking)

File header still says "10 indexes per master plan sec 5.1" but now creates 8. Inline breadcrumbs at lines 80-82 and 88-90 make the history clear, so not worth a follow-up.

Risk / reversibility

Risk: LOW. Pure DDL idempotent migration; no query plans depend on the removed names; canonical indexes (009.houses_geom_idx, 028.listing_sources_listing_idx) cover the same access paths.
Reversibility: trivial revert via git if needed.
Merge window: any time (no schema lock; CREATE/DROP INDEX of dupes are short-lived ops on houses and listing_sources tables).

## Deep Code Review — verdict: APPROVE ### Summary - Status: APPROVE - Files: 1 (P0: 1 — SQL migration) - Lines: +4 / -6 - Scope: removes 2 redundant `CREATE INDEX IF NOT EXISTS` from `050_search_optimization.sql` to close the PR #500 regression loop ### Claim verification (all confirmed) - `listing_sources_listing_idx` is canonical in `028_matching_tables.sql:71` (btree on `listing_id`) — single-column, exact duplicate of removed `_idx2`. - `houses_geom_idx` is canonical in `009_houses.sql:58` (GIST on geom, non-partial) — strictly redundant with removed `_idx2`. - `035_drop_duplicate_indexes.sql` drops both `_idx2` variants via `DROP INDEX IF EXISTS` (idempotent). - Migration sort order `028 → 035 → 050` confirmed via repo listing — alphanumeric sort puts 035 strictly before 050. Steady state after this PR: 028 creates canonical → 035 drops dupes (no-op on subsequent runs) → 050 no longer recreates them. Production dupes vanish on next deploy. ### Idempotency / transactional safety - Both `BEGIN; ... COMMIT;` blocks (indexes + matview) intact. - All remaining `CREATE INDEX IF NOT EXISTS` preserved → file re-run safe. - `deploy-tradein.yml` runs `for f in $(ls *.sql | sort); do psql -v ON_ERROR_STOP=on < $f; done` — sort + ON_ERROR_STOP both honored. No deploy script changes needed. ### Regression check - Repo-wide grep `listing_sources_listing_idx2|houses_geom_idx2` → only `050` (being removed) and `035` (dropping). Zero code references — no query, service, or test depends on the `_idx2` names. - `tradein-mvp/scripts/probe-migration-028.py:21` checks for canonical `listing_sources_listing_idx` only — unaffected. - 035 retained as durable DROP guard — correct strategy, since older deploys may still have dupes in prod that 050 had been re-creating until now. - The other `_idx2`-suffixed indexes in 050 (`listing_sources_price_divergence_idx2`, `houses_fingerprint_idx2`, `houses_kadastr_idx2`) are NOT removed and NOT dropped by 035 — confirmed they're unique (no canonical equivalent in earlier migrations). Suffix is just legacy naming, not duplicate. Correct call to leave them alone. ### Architecture / archaeology - Replacing the removed `CREATE INDEX` lines with breadcrumb comments pointing to `028_matching_tables.sql` and `009_houses.sql` is the right call — future readers won't be confused why 050 "skips" indexes 8 and 10. Good hygiene. - Removal happens at the source (050), not by adding another DROP — minimal blast radius, no migration churn. ### Tiny nit (non-blocking) - File header still says "10 indexes per master plan sec 5.1" but now creates 8. Inline breadcrumbs at lines 80-82 and 88-90 make the history clear, so not worth a follow-up. ### Risk / reversibility - Risk: LOW. Pure DDL idempotent migration; no query plans depend on the removed names; canonical indexes (`009.houses_geom_idx`, `028.listing_sources_listing_idx`) cover the same access paths. - Reversibility: trivial revert via git if needed. - Merge window: any time (no schema lock; CREATE/DROP INDEX of dupes are short-lived ops on `houses` and `listing_sources` tables).