Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dont duplicate CHECK constraints and DEFAULTs when altering column type #349

Merged
merged 4 commits into from
May 8, 2024

Conversation

andrew-farries
Copy link
Collaborator

@andrew-farries andrew-farries commented May 3, 2024

When a column is duplicated with a new type, check constraints and defaults defined on the column may be incompatible with the new type.

In this case column duplication should not fail, but rather the incompatible constraints and defaults should be ignored and not be duplicated to the new column.

This PR changes column duplication to ignore errors on DEFAULT and CHECK constraint duplication that look as though they are caused by a change of column type.

Fixes #348

Copy link
Member

@exekias exekias left a comment

Choose a reason for hiding this comment

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

I understand this doesn't affect other constraints, like UNIQUE or NOT NULL right?

@exekias
Copy link
Member

exekias commented May 3, 2024

I'm wondering if this will be unexpected for compatible types, ie when I cast from integer to int8 or float 🤔

@andrew-farries
Copy link
Collaborator Author

andrew-farries commented May 3, 2024

I understand this doesn't affect other constraints, like UNIQUE or NOT NULL right?

That's right, only defaults and check constraints are affected

@andrew-farries
Copy link
Collaborator Author

andrew-farries commented May 3, 2024

I'm wondering if this will be unexpected for compatible types, ie when I cast from integer to int8 or float 🤔

That was my concern too.

If we had done this before supporting multiple sub-operations in one alter column statement it would probably not have been the right decision.

Now however if you change type to a compatible type you can recreate the DEFAULT and CHECK in the same alter as the type change:

{
  "name": "35_alter_column_multiple",
  "operations": [
    {
      "alter_column": {
        "table": "users",
        "column": "age",
        "type": "bigint",
        "default": "0",
        "check": {
          "name": "age_check",
          "constraint": "age > 21"
        },
        "up": "...",
        "down": "..."
      }
    }
  ]
}

which mitigates the problem, although it might still be unexpected that you have to do this.

Ideally we would try to preserve checks and defaults iff they are compatible with the new type.

@andrew-farries andrew-farries marked this pull request as draft May 7, 2024 09:55
@andrew-farries
Copy link
Collaborator Author

We should preserve defaults and check constraints when they are compatible.

Putting this back into draft in order to do that.

@andrew-farries andrew-farries force-pushed the dont-duplicate-incompatible-constraints branch from c7b765d to 549d0cf Compare May 7, 2024 10:19
@andrew-farries andrew-farries marked this pull request as ready for review May 7, 2024 10:31
@andrew-farries
Copy link
Collaborator Author

Updated this to preserve DEFAULT and CHECK constraints when they are compatible, and ignore errors on duplication of DEFAULT and CHECK constraints when the error code suggests the failure is a result of column type change.

When a column is duplicated with a new type, check constraints and
defaults defined on the column *may* be incompatible with the new type.

In this case column duplication should not fail, but rather the
incompatible constraints and defaults should be ignored and not be
duplicated to the new column.
When changing a column type, incompatible defaults should be removed
from the column.
When changing a column type, incompatible checks should be removed
from the column.
@andrew-farries andrew-farries force-pushed the dont-duplicate-incompatible-constraints branch from 549d0cf to 54e2763 Compare May 8, 2024 15:01
@andrew-farries andrew-farries merged commit 4c1bc6a into main May 8, 2024
44 checks passed
@andrew-farries andrew-farries deleted the dont-duplicate-incompatible-constraints branch May 8, 2024 17:52
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.

Altering a column type attempts to duplicate incompatible defaults and constraints
2 participants