xataio / pgroll

PostgreSQL zero-downtime migrations made easy
https://www.xata.io
Apache License 2.0
2.97k stars 54 forks source link

Dont duplicate `CHECK` constraints and `DEFAULT`s when altering column type #349

Closed andrew-farries closed 4 months ago

andrew-farries commented 4 months ago

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 https://github.com/xataio/pgroll/issues/348

exekias commented 4 months ago

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

andrew-farries commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

Putting this back into draft in order to do that.

andrew-farries commented 4 months ago

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.