Revert "fix: preserve column order in UNIQUE constraints (#17)"

[tianzhou]

This reverts commit 109ca81.

#17

[Copilot]

Pull Request Overview

This PR reverts a previous fix that preserved column order in UNIQUE constraints, effectively removing the position-based sorting functionality for constraint columns.

• Removes the requiresPositionSorting function that determined which constraint types needed position-based sorting
• Eliminates position-based sorting for constraint columns and referenced columns in foreign keys
• Removes associated comments explaining the column ordering behavior

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

The comment on line 524 references 'position-based sorting' but the position-based sorting code has been removed. This comment should be updated to reflect the current behavior or removed entirely.

Suggested change

	// For partitioned tables, ensure primary key columns are ordered with partition key first
	// For partitioned tables, handle primary key columns as needed

[tianzhou]

@screenfluent I have taken a closer look today. And I think the original code has already handled this case https://github.com/pgschema/pgschema/blob/109ca81eb514570f443c7c12cd0a2cad1e8cf988/internal/diff/table.go#L29

I also ran the test cases in #17 (comment) and #17 (comment) before #17, and they are working as expected.

Our existing test case should have caught this:

PGSCHEMA_TEST_FILTER="create_table/add_table_composite_keys" go test -v ./cmd -run TestPlanAndApply

The reason is our underlying dump implementation is similar to migrate.

The dump is a diff between an empty schema and the target database schema. And it will go through the same diff logic.

[tianzhou]

@screenfluent I am going to revert #17 for now. But I am happy to take another look if there are things that I have overlooked.

[screenfluent]

@tianzhou hey I think there's a misunderstanding here - the bug is real and I can reproduce it right now.

The sortConstraintColumnsByPosition in table.go doesn't help with dump command because the data comes already wrong from inspector.BuildIR().

Here's quick test:

# In PostgreSQL:
psql -c "SELECT pg_get_constraintdef(oid) FROM pg_constraint WHERE conname = 'account_provider_unique';"
# Shows: UNIQUE ("providerId", "accountId")

# pgschema dump after revert:
pgschema dump --db directory_dev | grep "UNIQUE.*account"  
# Shows: UNIQUE ("accountId", "providerId")  <- wrong order!

The problem is dump workflow goes like this:

1. inspector.BuildIR() builds from database - already wrong order here
2. diff.GenerateMigration() compares empty vs full schema
3. sortConstraintColumnsByPosition runs but data is already wrong

So the sort in table.go can't fix what inspector gives it wrong in first place.

I can make a test specifically for dump command if that helps? The existing tests work because they test diff between SQL files not dump from real database.

[tianzhou]

@screenfluent, a test would be great! Don't worry about where to put the test. I can always adjust it later if needed.

[screenfluent]

@tianzhou here's the test! Put it wherever you think fits best.

func TestDumpPreservesConstraintColumnOrder(t *testing.T) {
    // Setup test database with UNIQUE (provider_id, account_id)
    db.Exec(`CREATE TABLE test_order (
        id text PRIMARY KEY,
        provider_id text NOT NULL,
        account_id text NOT NULL,
        CONSTRAINT test_unique UNIQUE (provider_id, account_id)
    )`)
    
    // Build IR from database
    inspector := ir.NewInspector(db)
    schema, _ := inspector.BuildIR(ctx, "public")
    
    // Check constraint column order
    constraint := schema.Schemas["public"].Tables["test_order"].Constraints["test_unique"]
    
    // This fails without the fix - columns are in alphabetical order
    assert.Equal(t, "provider_id", constraint.Columns[0].Name)
    assert.Equal(t, "account_id", constraint.Columns[1].Name)
}

The test fails without sorting columns by Position in buildConstraints().

Let me know if you need anything else!

[tianzhou]

@screenfluent I noticed the test case missed diff.GenerateMigration which is called by the dump command.

https://github.com/pgschema/pgschema/blob/69b101dea61a5d0b21111123cefea0401ab39653/cmd/dump/dump.go#L93

That's where we handle position sorting

https://github.com/pgschema/pgschema/blob/109ca81eb514570f443c7c12cd0a2cad1e8cf988/internal/diff/table.go#L29

[screenfluent]

I see what you mean, but the problem is that sortConstraintColumnsByPosition is NOT called for CREATE TABLE statements!

Here's the issue flow:

1. Dump calls GenerateMigration ✅ (line dump.go:93)
2. GenerateMigration calls generateCreateTablesSQL ✅ (diff.go:789)
3. generateCreateTablesSQL calls generateTableSQL ✅ (table.go:307)
4. generateTableSQL calls generateConstraintSQL ✅ (table.go:438)
5. generateConstraintSQL does NOT sort columns ❌ (constraint.go:14-19)

Look at generateConstraintSQL in constraint.go:

getColumnNames := func(columns []*ir.ConstraintColumn) []string {
    var names []string
    for _, col := range columns {  // <-- No sorting here!
        names = append(names, util.QuoteIdentifier(col.Name))
    }
    return names
}

Meanwhile, sortConstraintColumnsByPosition is only used in:

• generateAddConstraintSQL (for ALTER TABLE ADD CONSTRAINT)
• generateDropConstraintSQL (for ALTER TABLE DROP CONSTRAINT)

But NOT in generateConstraintSQL which is used for inline constraints in CREATE TABLE!

Here's proof the bug exists:

# Database has correct order:
psql -c "SELECT pg_get_constraintdef(oid) FROM pg_constraint WHERE conrelid =
'account'::regclass AND contype = 'u';"
# Output: UNIQUE ("providerId", "accountId")

# But dump shows wrong order:
pgschema dump --db directory_dev | grep "UNIQUE.*provider"
# Output: UNIQUE ("accountId", "providerId")  <-- Wrong!

The Position values ARE correctly loaded from database (providerId=1, accountId=2), but generateConstraintSQL doesn't use them.

The fix is either:

1. Sort by Position in inspector.BuildIR() (my original PR fix: preserve column order in UNIQUE constraints #17)
2. Or add sorting in generateConstraintSQL too