Repository navigation
Conversation
|
| 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$ |
There was a problem hiding this comment.
Is this pgschema_restore_0 needed?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This seems to generate extra schema beyond what user defines
There was a problem hiding this comment.
Agreed. I'll address the unnecessary restoration blocks shortly, while preserving index names where restoration is needed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🟡 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
regclassremapping 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.
|
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. |
|
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. |
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.
23d2e6b to
6a62073
Compare
|
@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? |

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
PostgreSQL names the inherited primary-key index
events_leaf_pkey1because the standalone parent index already occupiesevents_leaf_pkey.Without this fix, pgschema creates partition tables before standalone parent indexes during replay. PostgreSQL therefore assigns
events_leaf_pkeyto the inherited primary-key index. PostgreSQL skips the intended standalone parent index when executing the laterCREATE INDEX IF NOT EXISTS, and rejectsATTACH PARTITIONwith SQLSTATE42809.Changes
ALTER TABLE ONLY ... ADD CONSTRAINT, then emits exactALTER INDEX ... ATTACH PARTITIONstatements. PostgreSQL creates ordinary and custom names directly, preserving constraint ownership without rename blocks.regclassliterals inside executableDObodies. It preserves other code, aliases, record fields, ordinary strings and destination index names. External planning includes explicitly created indexes while preserving unrelated external references.U&,UESCAPEand continued Unicode literals. It rejects invalid Unicode and UTF-8 before rewriting and preserves native literal semantics.PartitionIndexDependencyErrorwitherrors.As, including after wrapping. The error retains contextual object names.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
go test -p 1 ./... -timeout=30m.psqlreplay 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.go vet ./..., build, formatting andgit diff --checkpassed.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.