fix: use DROP INDEX IF EXISTS in online index rebuild (#509)

[kenchan0130]

Fixes #509.

Problem

The online index-rebuild path emits a bare DROP INDEX <name>. When the same plan also drops a column the old index depends on, PostgreSQL implicitly drops that index as part of ALTER TABLE ... DROP COLUMN, so the later bare DROP INDEX fails with index "..." does not exist (SQLSTATE 42704).

The non-online path (internal/diff/index.go, generateIndexModifications) already guards this with DROP INDEX IF EXISTS; only the online rewrite path was affected. This is the index-side sibling of #384 (fix: skip constraint drop when DROP COLUMN already removes it, v1.9.0).

Fix

internal/plan/rewrite.go: change the two rebuild drop steps (generateIndexChangeRewrite, generateIndexChangeRewriteFromIndex) from DROP INDEX %s to DROP INDEX IF EXISTS %s, matching the existing non-online convention.

Reproduction (new test fixture)

testdata/diff/online/issue_509_index_rebuild_dropped_column — a partial index whose predicate column (valid_period) is dropped while a same-named index is redefined. It is not temporal-specific and reproduces on PostgreSQL 14–18.

Before the fix, TestPlanAndApply fails on this case with:

ERROR: index "idx_notifications_tenant_user" does not exist (SQLSTATE 42704)

After the fix it passes end-to-end.

Tests

• PGSCHEMA_TEST_FILTER="online/" go test ./cmd -run TestPlanAndApply — PASS
• PGSCHEMA_TEST_FILTER="online/" go test ./internal/diff -run TestDiffFromFiles — PASS

Regenerated the two existing online fixtures whose plan output changed (alter_composite_index, alter_materialized_view_index).

🤖 Generated with Claude Code

https://claude.ai/code/session_01RtS1QNgsBYgUr7CRXfdm8q

[greptile-apps]

Greptile Summary

This PR fixes a failure in the online index-rebuild path where DROP INDEX <name> would error with SQLSTATE 42704 when a same-plan DROP COLUMN step had already implicitly removed the index. The non-online path in internal/diff/index.go already used DROP INDEX IF EXISTS; the online path in rewrite.go was the only place that didn't.

• Core fix (internal/plan/rewrite.go): both generateIndexChangeRewrite and generateIndexChangeRewriteFromIndex now emit DROP INDEX IF EXISTS %s instead of DROP INDEX %s, matching the non-online convention and eliminating the 42704 failure.
• Test coverage: a new fixture (issue_509_index_rebuild_dropped_column) adds a partial-index/drop-column reproduction case; the two existing online fixtures (alter_composite_index, alter_materialized_view_index) are regenerated to reflect the updated SQL.

Confidence Score: 5/5

Safe to merge — the change is a two-character addition (IF EXISTS) in the online rebuild path, directly mirrors the already-proven non-online path, and is covered by both updated and new test fixtures.

The fix is minimal and surgical: two DROP INDEX statements gain IF EXISTS, matching the identical guard already present in the non-online diff path. The new test fixture exercises the exact failure scenario end-to-end, and the two regenerated fixtures confirm no other plan output regressed. No logic outside the SQL string construction was altered.

No files require special attention.

Important Files Changed

Filename	Overview
internal/plan/rewrite.go	Two DROP INDEX statements updated to DROP INDEX IF EXISTS in generateIndexChangeRewrite and generateIndexChangeRewriteFromIndex, with clear explanatory comments; change is minimal and correct.
testdata/diff/online/issue_509_index_rebuild_dropped_column/plan.json	New fixture plan: DROP COLUMN in group 1, concurrent CREATE in group 2, wait in group 3, DROP IF EXISTS + RENAME in group 4 — execution order is correct.
testdata/diff/online/alter_composite_index/plan.json	Existing fixture regenerated: DROP INDEX statements updated to DROP INDEX IF EXISTS to match the new rewrite.go output.
testdata/diff/online/alter_materialized_view_index/plan.json	Existing fixture regenerated: DROP INDEX statement updated to DROP INDEX IF EXISTS for the materialized view index case.
testdata/diff/online/issue_509_index_rebuild_dropped_column/old.sql	New test fixture: partial index on (tenant_id, user_id) WHERE valid_period IS NOT NULL — correctly represents the pre-condition for the bug (predicate column later dropped).

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant PG as PostgreSQL
    participant Plan as pgschema Plan Executor

    Note over Plan,PG: Transaction Group 1
    Plan->>PG: ALTER TABLE notifications DROP COLUMN valid_period
    PG-->>Plan: OK (implicitly drops idx_notifications_tenant_user)

    Note over Plan,PG: Transaction Group 2 (outside transaction)
    Plan->>PG: CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_..._pgschema_new ON notifications (tenant_id, user_id)
    PG-->>Plan: OK

    Note over Plan,PG: Transaction Group 3
    Plan->>PG: "SELECT ... WHERE c.relname = 'idx_..._pgschema_new' (wait)"
    PG-->>Plan: "done=true"

    Note over Plan,PG: Transaction Group 4
    Plan->>PG: DROP INDEX IF EXISTS idx_notifications_tenant_user
    Note right of PG: IF EXISTS silences 42704 — index already gone from Group 1
    PG-->>Plan: OK (no-op)
    Plan->>PG: ALTER INDEX idx_..._pgschema_new RENAME TO idx_notifications_tenant_user
    PG-->>Plan: OK

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant PG as PostgreSQL
    participant Plan as pgschema Plan Executor

    Note over Plan,PG: Transaction Group 1
    Plan->>PG: ALTER TABLE notifications DROP COLUMN valid_period
    PG-->>Plan: OK (implicitly drops idx_notifications_tenant_user)

    Note over Plan,PG: Transaction Group 2 (outside transaction)
    Plan->>PG: CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_..._pgschema_new ON notifications (tenant_id, user_id)
    PG-->>Plan: OK

    Note over Plan,PG: Transaction Group 3
    Plan->>PG: "SELECT ... WHERE c.relname = 'idx_..._pgschema_new' (wait)"
    PG-->>Plan: "done=true"

    Note over Plan,PG: Transaction Group 4
    Plan->>PG: DROP INDEX IF EXISTS idx_notifications_tenant_user
    Note right of PG: IF EXISTS silences 42704 — index already gone from Group 1
    PG-->>Plan: OK (no-op)
    Plan->>PG: ALTER INDEX idx_..._pgschema_new RENAME TO idx_notifications_tenant_user
    PG-->>Plan: OK

Loading

_{Reviews (1): Last reviewed commit: "fix: use DROP INDEX IF EXISTS in online ..." | Re-trigger Greptile}

[Copilot]

Pull request overview

Fixes the online (concurrent) index rebuild rewrite path to tolerate indexes that have already been implicitly dropped earlier in the same plan (e.g., by ALTER TABLE ... DROP COLUMN), avoiding SQLSTATE 42704 failures reported in #509.

Changes:

• Update online index-rebuild rewrite steps to emit DROP INDEX IF EXISTS ... (matching the non-online behavior).
• Add a new online diff fixture reproducing #509 with a partial index whose predicate column is dropped.
• Regenerate existing online fixtures whose planned SQL output changed due to the new IF EXISTS.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/plan/rewrite.go	Switch rewrite-generated index drop statements to DROP INDEX IF EXISTS for online rebuild flow.
testdata/diff/online/issue_509_index_rebuild_dropped_column/plan.txt	New fixture: expected human-readable plan output for #509 reproduction.
testdata/diff/online/issue_509_index_rebuild_dropped_column/plan.sql	New fixture: expected SQL plan output for #509 reproduction.
testdata/diff/online/issue_509_index_rebuild_dropped_column/plan.json	New fixture: expected JSON plan output for #509 reproduction.
testdata/diff/online/issue_509_index_rebuild_dropped_column/old.sql	New fixture: pre-change schema for #509 reproduction.
testdata/diff/online/issue_509_index_rebuild_dropped_column/new.sql	New fixture: post-change schema for #509 reproduction.
testdata/diff/online/issue_509_index_rebuild_dropped_column/diff.sql	New fixture: expected diff output for #509 reproduction.
testdata/diff/online/alter_materialized_view_index/plan.txt	Regenerated fixture to reflect DROP INDEX IF EXISTS in online plan output.
testdata/diff/online/alter_materialized_view_index/plan.sql	Regenerated fixture to reflect DROP INDEX IF EXISTS in online plan output.
testdata/diff/online/alter_materialized_view_index/plan.json	Regenerated fixture to reflect DROP INDEX IF EXISTS in online plan output.
testdata/diff/online/alter_composite_index/plan.txt	Regenerated fixture to reflect DROP INDEX IF EXISTS in online plan output.
testdata/diff/online/alter_composite_index/plan.sql	Regenerated fixture to reflect DROP INDEX IF EXISTS in online plan output.
testdata/diff/online/alter_composite_index/plan.json	Regenerated fixture to reflect DROP INDEX IF EXISTS in online plan output.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kenchan0130]

Valid concern, but this is pre-existing, codebase-wide behavior rather than something introduced here, so I've kept it as-is.

qualifyEntityName(schema, name, targetSchema) returns a bare name when schema == targetSchema (internal/diff/diff.go:2175). Managed indexes are always in the target schema, so the non-online path (generateIndexModifications) already emits the same unqualified DROP INDEX IF EXISTS <bare-name> — visible in this PR's own diff.sql fixture. And apply sets SET search_path TO <target>, public (cmd/apply/apply.go:231): the whole codebase intentionally relies on search_path inside the target schema instead of schema-qualifying. This change just makes the online rewrite path consistent with that convention.

The suggested rule ("qualify when non-public") differs from the codebase rule ("qualify only when entitySchema != targetSchema"); applying it only here would diverge from generateIndexModifications and leave the identical exposure in the non-online path. Hardening against a same-named public index shadowing a target-schema object is a separate, broader change that should cover both paths uniformly — out of scope for this focused #509 regression fix. Happy to open a follow-up if maintainers want it.

[kenchan0130]

Same as the reply on the generateIndexChangeRewriteFromIndex drop above: this mirrors the already-shipping non-online path, which also emits an unqualified DROP INDEX IF EXISTS for target-schema indexes (qualifyEntityName returns a bare name when schema == targetSchema, diff.go:2175) and relies on the search_path set by apply (cmd/apply/apply.go:231). Keeping it unqualified here makes the online rewrite path consistent with that convention. A uniform hardening across both paths would be a separate follow-up, out of scope for this #509 regression fix.

[kenchan0130]

Thanks for the review via Copilot. I've responded inline to the two DROP INDEX IF EXISTS comments.

In short: the unqualified index name is pre-existing, codebase-wide behavior (the non-online path emits the same statement, and apply relies on search_path), and this PR keeps the online rewrite path consistent with that convention rather than introducing a new bug. Hardening against a same-named index in public would be a separate, broader change spanning both paths — out of scope for this focused #509 regression fix. Details in the inline replies.

[tianzhou]

LGTM. Thanks for the contribution.