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

Implement the drop_constraint operation #103

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

andrew-farries
Copy link
Collaborator

Implement the drop_constraint operation for dropping constraints defined on single columns.

An example of the operation looks like:

{
  "name": "23_drop_check_constraint",
  "operations": [
    {
      "drop_constraint": {
        "table": "posts",
        "column": "title",
        "name": "title_length",
        "up": "title",
        "down": "(SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END)"
      }
    }
  ]
}

for dropping a CHECK constraint. And like this for dropping a FOREIGN KEY constraint:

{
  "name": "24_drop_foreign_key_constraint",
  "operations": [
    {
      "drop_constraint": {
        "table": "posts",
        "column": "user_id",
        "name": "fk_users_id",
        "up": "user_id",
        "down": "(SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END)"
      }
    }
  ]
}

The operation works very similarly to the inverse operation of adding CHECK and FOREIGN KEY constraints to a single column.

  • On Start:
    • a new column without the constraint is added to the underlying table.
    • triggers are created using the up and down SQL. The down SQL needs to ensure that rows inserted into the new view that don't meet the constraint are converted into rows that do meet the constraint.
  • On Complete
    • Triggers are removed, the old column is deleted and the new column is renamed.
  • On Rollback
    • The new column and the triggers are removed.

Improvements

  • The drop_constraint operation requires that the column on which the constraint is defined is named in the migration json file. If pg-roll's internal schema representation knew about the constraints defined on a table it would be possible to delete constraints by constraint name only; the schema representation would know on which column the constraint was defined.
  • pg-roll currently only allows for creating CHECK and FOREIGN KEY constraints on single columns; this limitation also applies to the drop_constraint operation.

Comment on lines +151 to +157
if o.Up == "" {
return FieldRequiredError{Name: "up"}
}

if o.Down == "" {
return FieldRequiredError{Name: "down"}
}
Copy link
Member

Choose a reason for hiding this comment

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

dropping a constraint is in many cases an easy operation, you land on a less restrictive schema but the old one keeps working. I wonder if these should be optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the case of removing a constraint the down SQL is the important one as that's the one that goes from a less restrictive schema to a more restrictive one. In the absence of data quarantine, it's necessary to ensure that values inserted into the new schema can successfully be written to the old one.

Theup sql could probably be made optional as it's almost always going to be a straight copy of the value from the more restrictive schema to the less restrictive one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PR here to make the up SQL optional: #106

@exekias
Copy link
Member

exekias commented Sep 18, 2023

  • If pg-roll's internal schema representation knew about the constraints defined on a table it would be possible to delete constraints by constraint name only; the schema representation would know on which column the constraint was defined.

I believe we will need this anyway, for instance to validate that the migration makes sense (the constraint exists?). Also, if you think about exposing postgres schema to Xata users, probably our internal representation is a great candidate for this.

@andrew-farries
Copy link
Collaborator Author

  • If pg-roll's internal schema representation knew about the constraints defined on a table it would be possible to delete constraints by constraint name only; the schema representation would know on which column the constraint was defined.

I believe we will need this anyway, for instance to validate that the migration makes sense (the constraint exists?).

Agreed, it would be useful for migration validation if nothing else.

Also, if you think about exposing postgres schema to Xata users, probably our internal representation is a great candidate for this.

You mean as an alternative to giving access to system catalogs?

@andrew-farries
Copy link
Collaborator Author

andrew-farries commented Sep 18, 2023

I've created #105 for adding constraints to the internal schema representation.

@exekias
Copy link
Member

exekias commented Sep 18, 2023

You mean as an alternative to giving access to system catalogs?

No, I was thinking about exposing some form of processed schema in our API, for some clients to consume, let's discuss this at some point

@andrew-farries andrew-farries merged commit 31402f9 into main Sep 19, 2023
7 checks passed
@andrew-farries andrew-farries deleted the drop-constraint-operation branch September 19, 2023 04:32
andrew-farries added a commit that referenced this pull request Sep 19, 2023
In response to this comment
#103 (comment).

Theup sql is made optional as it's almost always going to be a straight
copy of the value from the more restrictive schema to the less
restrictive one.
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