fix: Postgres Drop() also removes custom enum types#1391
Conversation
Drop() only drops tables, leaving custom types (enums) behind. When a migration creates a custom type, Drop() followed by Up() fails with 'type already exists'. Add type dropping after table dropping by querying pg_type for enum types in the current schema. Fixes golang-migrate#626
There was a problem hiding this comment.
Pull request overview
Updates the Postgres driver’s Drop() implementation to fully clean a schema by removing custom enum types in addition to dropping tables, fixing the “type already exists” failure when re-running migrations after Drop().
Changes:
- Query
pg_type/pg_namespacefor enum types in the current schema (typtype = 'e'). - Iterate found enum types and
DROP TYPE IF EXISTS ... CASCADEafter tables are dropped.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| query = `DROP TYPE IF EXISTS ` + pq.QuoteIdentifier(typeName) + ` CASCADE` | ||
| if _, err := p.conn.ExecContext(context.Background(), query); err != nil { | ||
| return &database.Error{OrigErr: err, Query: []byte(query)} | ||
| } | ||
| } | ||
| if err := types.Err(); err != nil { | ||
| return &database.Error{OrigErr: err, Query: []byte(query)} | ||
| } |
There was a problem hiding this comment.
types.Err() is wrapped using the mutable query variable, but by the time this runs query may contain the last DROP TYPE ... statement rather than the original SELECT ... FROM pg_type .... This makes the reported query misleading and complicates debugging. Consider keeping the SELECT in a separate variable (e.g., typesQuery) and using that for Query: in errors coming from iteration/scan/Err(), while using a separate dropQuery for the DDL.
| for types.Next() { | ||
| var typeName string | ||
| if err := types.Scan(&typeName); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
On types.Scan(&typeName) failure, this returns the raw error without attaching the SQL that was executing. Elsewhere in this method, query errors are wrapped with database.Error{..., Query: ...}; doing the same here (using the SELECT query, not the last DROP) would improve error context consistency.
| for types.Next() { | ||
| var typeName string | ||
| if err := types.Scan(&typeName); err != nil { | ||
| return err | ||
| } | ||
| query = `DROP TYPE IF EXISTS ` + pq.QuoteIdentifier(typeName) + ` CASCADE` | ||
| if _, err := p.conn.ExecContext(context.Background(), query); err != nil { | ||
| return &database.Error{OrigErr: err, Query: []byte(query)} | ||
| } | ||
| } | ||
| if err := types.Err(); err != nil { | ||
| return &database.Error{OrigErr: err, Query: []byte(query)} | ||
| } | ||
|
|
There was a problem hiding this comment.
This new behavior (dropping enum types during Drop()) isn't covered by the existing Postgres driver tests; database/testing.TestDrop only checks that Drop() returns nil. Please add an integration test that creates a custom enum type in the current schema, calls Drop(), and asserts the type no longer exists (or that re-creating the type succeeds) to prevent regressions like #626.
| for types.Next() { | |
| var typeName string | |
| if err := types.Scan(&typeName); err != nil { | |
| return err | |
| } | |
| query = `DROP TYPE IF EXISTS ` + pq.QuoteIdentifier(typeName) + ` CASCADE` | |
| if _, err := p.conn.ExecContext(context.Background(), query); err != nil { | |
| return &database.Error{OrigErr: err, Query: []byte(query)} | |
| } | |
| } | |
| if err := types.Err(); err != nil { | |
| return &database.Error{OrigErr: err, Query: []byte(query)} | |
| } | |
| typeNames := make([]string, 0) | |
| for types.Next() { | |
| var typeName string | |
| if err := types.Scan(&typeName); err != nil { | |
| return err | |
| } | |
| if len(typeName) > 0 { | |
| typeNames = append(typeNames, typeName) | |
| } | |
| } | |
| if err := types.Err(); err != nil { | |
| return &database.Error{OrigErr: err, Query: []byte(query)} | |
| } | |
| if len(typeNames) > 0 { | |
| for _, t := range typeNames { | |
| query = `DROP TYPE IF EXISTS ` + pq.QuoteIdentifier(t) + ` CASCADE` | |
| if _, err := p.conn.ExecContext(context.Background(), query); err != nil { | |
| return &database.Error{OrigErr: err, Query: []byte(query)} | |
| } | |
| } | |
| } |
Fixes #626
Problem
Drop()only drops tables but leaves custom types (enums, etc.) behind. When a migration creates a custom type:Running
Drop()followed byUp()fails:Fix
After dropping tables, query
pg_typefor enum types (typtype = 'e') in the current schema and drop them withCASCADE. This follows the same pattern used for table dropping (iterate +DROP IF EXISTS).Previous PR #477 was closed without merge. This implementation uses a simpler approach querying
pg_typedirectly.