Skip to content

Conversation

@sam-mosleh
Copy link
Contributor

Problem Statement

When adding and removing NOT NULL constraints, pgschema fails to properly clean up temporary constraints. This leads to constraint name collisions and migration failures when attempting to re-add the constraint.

Error Message

ERROR: constraint "age_not_null" for relation "users" already exists (SQLSTATE 42710)

Root Cause

pgschema creates named constraints when NOT NULL is added to a column. When the constraint is removed in a subsequent migration, the temporary constraint object is not being properly dropped from the database. This orphaned constraint prevents future migrations from creating a new constraint with the same name.

Reproduction Steps

The issue can be reproduced with the following sequence of schema changes:

-- Step #1: Create table without constraint
CREATE TABLE users (
    id int PRIMARY KEY,
    age int
);

-- Step #2: Add NOT NULL constraint
CREATE TABLE users (
    id int PRIMARY KEY,
    age int NOT NULL
);

-- Step #3: Remove NOT NULL constraint
CREATE TABLE users (
    id int PRIMARY KEY,
    age int
);

-- Step #4: Re-add NOT NULL constraint (FAILS)
CREATE TABLE users (
    id int PRIMARY KEY,
    age int NOT NULL
);

Expected Result: Step 4 should succeed
Actual Result: Error thrown due to constraint name collision

@sam-mosleh sam-mosleh changed the title 🐛 Properly Drop Temporary NOT NULL Constraints fix: properly drop temporary NOT NULL constraint Nov 18, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where temporary CHECK constraints used during NOT NULL column migrations were not being properly cleaned up, causing constraint name collisions on subsequent migrations.

Key Changes:

  • Added a 4th step to the NOT NULL migration workflow that drops the temporary CHECK constraint after setting the column to NOT NULL
  • Updated test data to reflect the new cleanup step

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
internal/plan/rewrite.go Added logic to generate and execute DROP CONSTRAINT statement for temporary CHECK constraint cleanup
testdata/diff/online/add_not_null/plan.txt Updated expected migration plan output to include constraint cleanup
testdata/diff/online/add_not_null/plan.sql Updated expected SQL output to include DROP CONSTRAINT statement
testdata/diff/online/add_not_null/plan.json Updated expected JSON plan to include new cleanup step

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@tianzhou tianzhou left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

@tianzhou tianzhou merged commit a54bee6 into pgplex:main Nov 23, 2025
7 checks passed
@sam-mosleh sam-mosleh deleted the patch-1 branch November 24, 2025 11:28
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.

2 participants