Skip to content

fix: preserve inherited partition index names - #645

Open
zikolach wants to merge 4 commits into
pgplex:mainfrom
zikolach:fix/inherited-partition-index-names
Open

zikolach wants to merge 4 commits into
pgplex:mainfrom
zikolach:fix/inherited-partition-index-names

Conversation

@zikolach

@zikolach zikolach commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #653

Summary

Follow-up to #643. This PR preserves inherited constraint-index names during dump replay and complete new-hierarchy migrations. It also gives callers a stable type for partition-index dependency errors.

Reproduction

CREATE TABLE public.events (id int PRIMARY KEY) PARTITION BY RANGE (id);
CREATE INDEX events_leaf_pkey ON public.events (id);
CREATE TABLE public.events_leaf PARTITION OF public.events
    FOR VALUES FROM (0) TO (10);

PostgreSQL names the inherited primary-key index events_leaf_pkey1 because the standalone parent index already occupies events_leaf_pkey.

Without this fix, pgschema creates partition tables before standalone parent indexes during replay. PostgreSQL therefore assigns events_leaf_pkey to the inherited primary-key index. PostgreSQL skips the intended standalone parent index when executing the later CREATE INDEX IF NOT EXISTS, and rejects ATTACH PARTITION with SQLSTATE 42809.

Changes

  • The inspector records inherited PRIMARY KEY and UNIQUE index identities and catalog constraint definitions as transient metadata. These indexes remain under constraint management.
  • For complete new families within one table-creation batch, the generator creates named constraints with ALTER TABLE ONLY ... ADD CONSTRAINT, then emits exact ALTER INDEX ... ATTACH PARTITION statements. PostgreSQL creates ordinary and custom names directly, preserving constraint ownership without rename blocks.
  • For dependency-sensitive families or unsupported catalog metadata, the generator retains strict catalog-based restoration after partition creation. Two-phase renaming handles name swaps. Lookup and rename failures propagate to the caller.
  • The generator schedules constraints before dependent functions, foreign keys and policies. Multi-file dumps preserve dependency order. Index attachments and constraint comments retain their plan metadata and rendering.
  • The desired-state loader remaps explicit relation-valued regclass literals inside executable DO bodies. It preserves other code, aliases, record fields, ordinary strings and destination index names. External planning includes explicitly created indexes while preserving unrelated external references.
  • The loader handles PostgreSQL Unicode identifiers and strings, including U&, UESCAPE and continued Unicode literals. It rejects invalid Unicode and UTF-8 before rewriting and preserves native literal semantics.
  • Callers can classify PartitionIndexDependencyError with errors.As, including after wrapping. The error retains contextual object names.
  • Tests, documentation and affected dump snapshots cover the new behavior. The Sakila snapshot no longer contains restoration DO blocks.

Existing-table attachment reconciliation remains out of scope. Migration generation retains existing behavior for incomplete or filtered hierarchies. Ignored originating constraints remain ignored. Public IR JSON and fingerprints are unchanged.

Validation

  • Full PostgreSQL 18 suite passed using go test -p 1 ./... -timeout=30m.
  • Focused tests passed on PostgreSQL 14, 15, 16, 17 and 18, including PK/UNIQUE collisions, duplicate definitions, name swaps, nested partitions, ignored constraints, dependency-sensitive fallback, strict error propagation and transaction rollback.
  • Actual CLI dump, plan, native apply and psql replay passed for single-file/multi-file and qualified/unqualified output. Catalog comparisons confirmed exact names, definitions, parent links, locality, validity and ownership. Follow-up plans were empty through embedded and external desired-state providers.
  • Unicode, alias and record-field regressions passed through both providers. Actual CLI Unicode checks matched independently executed native PostgreSQL acceptance or rejection.
  • go vet ./..., build, formatting and git diff --check passed.
  • Independent correctness review returned Ready for the latest change.

All database validation used owned local PostgreSQL instances and synthetic schemas. No installed binary was replaced.

AI assistance was used for implementation, tests and review.

@greptile-apps

greptile-apps Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[High impact] Database schema migration and index management logic.

Fix index-valued regclass rewriting in external planning before merging.

Findings

  1. P1 External plans miss created indexes ▶

Summary

This PR records inherited constraint-index names and restores them immediately after partition creation.

  • Partition replay restores inherited index names before standalone indexes are created.
  • Desired SQL redirects explicit regclass literals inside DO bodies.
  • Partition-index dependency failures expose the missing objects as a typed error.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Inspect inherited constraint indexes] --> B[Create partition table]
  B --> C[Find inherited indexes in catalogs]
  C --> D[Move mismatched names to temporary names]
  D --> E[Restore source index names]
  E --> F[Create standalone indexes]
  F --> G[Attach recorded child indexes]
Loading

Reviews (1) · Last reviewed commit: "fix: preserve inherited partition index ..." · Reviewed by Greptile

Comment thread internal/postgres/desired_state.go Outdated
Comment thread testdata/dump/sakila/pgschema.sql Outdated
CONSTRAINT payment_p2022_05_rental_id_fkey FOREIGN KEY (rental_id) REFERENCES rental (rental_id),
CONSTRAINT payment_p2022_05_staff_id_fkey FOREIGN KEY (staff_id) REFERENCES staff (staff_id)
) FOR VALUES FROM ('2022-05-01 00:00:00+00') TO ('2022-06-01 00:00:00+00');
DO $pgschema_restore_0$

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.

Is this pgschema_restore_0 needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The block preserves inherited PRIMARY KEY and UNIQUE index names during replay. For example, if the source index was renamed from events_leaf_pkey to custom_leaf, PARTITION OF creates a new inherited index with a generated name. The block locates that index through its parent constraint and restores custom_leaf while preserving constraint ownership. The two rename passes also handle swapped names.

The generator currently emits the block for every eligible inherited index, including default names. When the generated name already matches, the block only checks catalog identity and performs no rename.

$pgschema_restore_0$ is the dollar-quote delimiter, chosen so it cannot occur inside the generated body.

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.

This seems to generate extra schema beyond what user defines

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll address the unnecessary restoration blocks shortly, while preserving index names where restoration is needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 23d2e6b. Eligible complete new families now create each PK/UNIQUE constraint with its exact source name, then attach its index to the recorded parent. The Sakila snapshot no longer contains restoration DO blocks.

For example, after creating the tables:

ALTER TABLE ONLY events_low
    ADD CONSTRAINT events_low_pkey PRIMARY KEY (id);
ALTER TABLE ONLY events
    ADD CONSTRAINT events_pkey PRIMARY KEY (id);
ALTER INDEX events_pkey ATTACH PARTITION events_low_pkey;

This uses the same path for ordinary and custom names, without predicting generated names or renaming indexes. PostgreSQL retains constraint ownership. Families split across dependency-sensitive creation stages, or lacking supported catalog metadata, retain the existing restoration path.

Validation passed on PostgreSQL 14–18 for focused acceptance and the full PostgreSQL 18 suite. Actual CLI dump/apply and psql replay passed for single/multi-file and qualified/unqualified output, including ordinary keys, custom names, nested partitions and name swaps. Catalogs matched and follow-up plans were empty.

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.

🟡 Changes recommended

The lexer corrupts valid PostgreSQL Unicode-escaped identifiers and misses Unicode DO bodies.

1 open finding
What changed in this PR

Preserves inherited partition constraint-index names during dump replay and complete hierarchy creation.

Changes:

  • Adds transient inherited-index metadata and restoration SQL.
  • Introduces typed partition dependency errors.
  • Adds DO-body regclass remapping and literal handling with extensive tests.
File Description
testdata/​dump/​sakila/​pgschema.sql Updates dump snapshot.
testdata/​dump/​issue_472_partition_clone_trigger/​pgschema.sql Updates partition snapshot.
testdata/​dump/​issue_409_partitioned_fk/​pgschema.sql Updates partitioned-FK snapshot.
ir/​partition_index.go Inspects inherited constraint indexes.
ir/​ir.go Defines transient index metadata.
ir/​index_test.go Tests metadata serialization exclusion.
internal/​postgres/​sql_literal.go Decodes PostgreSQL literals.
internal/​postgres/​sql_literal_test.go Tests literal semantics.
internal/​postgres/​sql_lex.go Adds SQL tokenization and DO remapping.
internal/​postgres/​external.go Propagates remapping errors.
internal/​postgres/​embedded.go Propagates remapping errors.
internal/​postgres/​do_remap_test.go Tests DO-body rewriting.
internal/​postgres/​desired_state.go Uses token-aware schema remapping.
internal/​postgres/​desired_state_test.go Updates remapping tests.
internal/​diff/​table.go Co-locates restoration with table creation.
internal/​diff/​partition_index.go Plans restoration and typed errors.
internal/​diff/​partition_index_test.go Tests typed dependency context.
internal/​diff/​partition_index_error_test.go Tests error wrapping.
internal/​diff/​inherited_constraint_index.go Generates index restoration SQL.
internal/​diff/​inherited_constraint_index_test.go Tests restoration eligibility.
docs/​cli/​plan.mdx Documents planning behavior.
docs/​cli/​dump.mdx Documents dump restoration.
cmd/​plan/​plan.go Normalizes transient schema references.
cmd/​plan/​plan_test.go Tests metadata normalization.
cmd/​inherited_constraint_index_test.go Tests round-trip restoration.
cmd/​inherited_constraint_index_ignore_test.go Tests ignored origins.
cmd/​inherited_constraint_index_collision_test.go Tests temporary-name collisions.
cmd/​dump/​partition_index_ignore_test.go Tests typed dump errors.
cmd/​do_regclass_desired_state_test.go Tests aliases and records.
cmd/​do_literal_desired_state_test.go Tests loader literal semantics.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/postgres/sql_lex.go
@tianzhou

Copy link
Copy Markdown
Contributor

This becomes too big to review, can you please open an issue with the problem first? I need to take a detailed look.

@zikolach

Copy link
Copy Markdown
Contributor Author

This becomes too big to review, can you please open an issue with the problem first? I need to take a detailed look.

Agreed. It is getting out of control. Just trying to make pgschema covering the "not so common" use cases to fully adopt it on my project. Don't want to implement workaround and stick to native experience.

Will think and try to summarize topics addressed in the PR.

@zikolach

Copy link
Copy Markdown
Contributor Author

Opened #653 with the minimal reproduction, failure modes, and expected behavior, as requested. The issue describes the problem independently of the implementation in this PR.

@zikolach
zikolach requested a review from tianzhou October 15, 2026 15:16
Restore constraint-owned index names before creating standalone indexes. Remap explicit regclass literals in DO blocks without changing aliases or record fields. Add typed partition-index dependency errors and replay coverage.
@zikolach
zikolach force-pushed the fix/inherited-partition-index-names branch from 23d2e6b to 6a62073 Compare October 16, 2026 14:57
@zikolach

Copy link
Copy Markdown
Contributor Author

@tianzhou thanks for your feedback! Could you please suggest what else I can do to make this PR accepted? Please tell me if you see any opportunities to simplify or split it? Or suggestions regarding how to implement it better?

This branch has not been deployed

No deployments
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.

Partitioned schema dumps can fail to replay or omit indexes when inherited index names change

3 participants