Skip to content

fix: keep the online index candidate inside the identifier limit - #679

Merged
tianzhou merged 1 commit into
pgplex:mainfrom
paoValle:fix/online-index-temp-name-length
Nov 1, 2026
Merged

tianzhou merged 1 commit into
pgplex:mainfrom
paoValle:fix/online-index-temp-name-length

Conversation

@paoValle

Copy link
Copy Markdown
Contributor

A 63-byte index name cannot be replaced through the online rewrite: name + "_pgschema_new" is 76 bytes, PostgreSQL truncates it back to the canonical name, CREATE INDEX CONCURRENTLY IF NOT EXISTS skips the build, the wait returns at once, and the final DROP/ALTER pair fails with 42P01.

onlineIndexTempName keeps the current name when the suffix fits (so lookup is still lookup_pgschema_new and no existing fixture moves) and otherwise shortens the canonical part and takes the tail from a SHA-256 of the full name: at most 63 bytes, never equal to the canonical name, distinct for names sharing a long prefix, and cut on a UTF-8 boundary so a multi-byte name cannot produce invalid UTF-8 for the server.

Both call sites use it (index replacement from the new IR index, and the IndexDiff path). The new testdata/diff/online/issue_670_long_index_name case is the issue's fixture with a 63-byte name; plan.sql was generated with -generate and re-checked with the integration suite in compare mode.

go build ./..., go vet ./internal/plan/ and go test ./... pass.

Closes #670

@greptile-apps

greptile-apps Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The change affects online index replacements across table and materialized-view indexes.

Add the missing diff.sql before merging so the new fixture passes the file-based diff test.

Findings

  1. P1 New diff test fails ▶

Summary

This PR keeps temporary online index names within PostgreSQL’s 63-byte limit.

  • Online index replacements use temporary names that fit PostgreSQL’s limit.

Reviews (1) · Last reviewed commit: "fix: keep the online index candidate ins..." · Reviewed by Greptile

CONSTRAINT entries_pkey PRIMARY KEY (id)
);

CREATE INDEX iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii ON entries (b);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 New diff test fails

The new fixture is missing diff.sql. TestDiffFromFiles visits this directory and reads that file unconditionally, so its new subtest fails on PostgreSQL 18 before comparing either schema. Add the expected non-online migration as diff.sql; the online output in plan.sql serves a different test.

Appending _pgschema_new to a 63-byte index name produced a 76-byte candidate
that PostgreSQL truncated back to the canonical name. CREATE INDEX
CONCURRENTLY IF NOT EXISTS then skipped the build, the wait returned at once,
and the final DROP/ALTER pair failed with 42P01.

The candidate now shortens the canonical part when the suffix does not fit and
takes the tail from a SHA-256 of the full name, so it is at most 63 bytes,
differs from the canonical name, and stays distinct for names that share a long
prefix. Names that fit are unchanged (lookup -> lookup_pgschema_new).

Closes pgplex#670
@paoValle
paoValle force-pushed the fix/online-index-temp-name-length branch from 2a30262 to 16b7d85 Compare October 30, 2026 09:11
@paoValle

Copy link
Copy Markdown
Contributor Author

That file is on the branch already. testdata/diff/online/issue_670_long_index_name/diff.sql was added in the amend that is now the PR head (16b7d85e), so the review was reading the earlier push (c0f4ccae → 2a302629 → 16b7d85e, the last two amends 18 and 38 minutes after that comment).

Against the embedded PostgreSQL 18.3.0:

PGSCHEMA_TEST_FILTER="online/issue_670_long_index_name" go test ./internal/diff -run TestDiffFromFiles -v
--- PASS: TestDiffFromFiles/online_issue_670_long_index_name (0.13s)
ok  github.com/pgplex/pgschema/internal/diff  7.077s

PGSCHEMA_TEST_FILTER="online/issue_670_long_index_name" go test ./cmd -run TestPlanAndApply -count=1 -v
--- PASS: TestPlanAndApply/online_issue_670_long_index_name (1.08s)
ok  github.com/pgplex/pgschema/cmd  46.161s

All 19 online/ subtests pass as well. Nothing further needed here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues were identified for the change that keeps temporary online-replacement index names within PostgreSQL’s 63-byte limit.

No open issues 🎉

What changed in this PR

Fixes #670 by keeping temporary online-replacement index names within PostgreSQL’s 63-byte identifier limit, preventing truncation from colliding with the original index.

Changes:

  • Adds a shared naming helper that preserves short names and shortens long names using a hash suffix and UTF-8-safe truncation.
  • Updates both online index replacement paths to use the helper.
  • Adds unit coverage and a regression fixture for a 63-byte index name.
File Description
testdata/​diff/​online/​issue_670_long_index_name/​plan.txt Expected readable online replacement plan.
testdata/​diff/​online/​issue_670_long_index_name/​plan.sql Expected SQL using the shortened candidate name.
testdata/​diff/​online/​issue_670_long_index_name/​plan.json Expected structured plan and transaction groups.
testdata/​diff/​online/​issue_670_long_index_name/​old.sql Original maximum-length index on column a.
testdata/​diff/​online/​issue_670_long_index_name/​new.sql Desired index on column b.
testdata/​diff/​online/​issue_670_long_index_name/​diff.sql Expected baseline index replacement DDL.
internal/​plan/​rewrite.go Adds bounded candidate naming to both replacement paths.
internal/​plan/​rewrite_test.go Tests naming limits, UTF-8 validity, and rewrite usage.

Review effort: Balanced

@tianzhou tianzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the contribution.

@tianzhou
tianzhou merged commit efce52b into pgplex:main Nov 1, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Online index replacement self-collides for a legal 63-byte index name

3 participants