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 set column NOT NULL operation #63

Merged
merged 9 commits into from
Aug 29, 2023
Merged

Conversation

andrew-farries
Copy link
Collaborator

@andrew-farries andrew-farries commented Aug 25, 2023

Implement the operation to make an existing column NOT NULL.

The migration looks like this:

{
  "name": "16_set_not_null",
  "operations": [
    {
      "set_not_null": {
        "table": "reviews",
        "column": "review",
        "up": "product || ' is good'"
      }
    }
  ]
}

This migration adds a NOT NULL constraint to the review column in the reviews table.

  • On Start:
    • Create a new column with a NOT VALID NOT NULL constraint
    • Backfill the new column with values from the existing column using the up SQL to replace NULL values
    • Create a trigger to populate the new column when values are written to the old column, rewriting NULLs with up SQL.
    • Create a trigger to populate the old column when values are written to the new column.
  • On Complete
    • Validate the NOT VALID NOT NULL constraint on the new column.
    • Add NOT NULL to the new column.
    • Remove triggers and the NOT VALID NOT NULL constraint
    • Drop the old column
    • Rename the new column to the old column name.
  • On Rollback
    • Remove the new column and both triggers.

Allow the trigger to take:
* an additional expression to test before applying the `up` or `down`
  SQL.
* An optional `else` clause

The extra test expresssion is useful when deciding whether to apply the
`up` SQL when writing to a column that should have `NOT NULL`
constraint. In this case the `up` SQL should be used only if the value
to be written is `NULL`.

The `else` clause is useful in the same situation when the value to be
written is not `NULL`; in this case the value should be written to the
new column unchanged.

The generated SQL looks like:

```
IF search_path <> latest_schema AND NEW."review" IS NULL THEN
  NEW."_pgroll_new_review" = product || ' is good';
ELSE
  NEW."_pgroll_new_review" = NEW."review";
END IF
```
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.

It's so exciting to see this working 😍

Left a couple of comments, I'm especially interested in your thoughts about up vs fallback/´backfill` operation

pkg/migrations/triggers.go Show resolved Hide resolved
pkg/migrations/op_set_notnull.go Show resolved Hide resolved
pkg/migrations/op_set_notnull.go Show resolved Hide resolved
pkg/migrations/op_set_notnull.go Outdated Show resolved Hide resolved
Comment on lines +190 to +191
// TODO: This function needs to be able to duplicate a column more precisely
// including constraints, indexes, defaults, etc.
Copy link
Member

Choose a reason for hiding this comment

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

very good point! Ideally we will have all the info needed for that in the schema object

Combine `NameRequiredError` and `UpSQLRequiredError` into a single
`FieldRequiredError` that takes the name of the missing field.
@andrew-farries andrew-farries merged commit 1da1d9b into main Aug 29, 2023
@andrew-farries andrew-farries deleted the set-not-null branch August 29, 2023 13:53
andrew-farries added a commit that referenced this pull request Aug 29, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Refactor the unwieldy `Sprintf`-based trigger generation code:


https://github.com/xataio/pg-roll/blob/main/pkg/migrations/triggers.go#L54-L78

With an equivalent approach that uses text templates:

```go
package templates

const Function = `CREATE OR REPLACE FUNCTION {{ .Name | qi }}()
    RETURNS TRIGGER
    LANGUAGE PLPGSQL
    AS $$
    DECLARE
      {{- $schemaName := .SchemaName  }}
      {{- $tableName := .TableName  }}
      {{ range .Columns }} 
      {{- .Name | qi }} {{ $schemaName | qi }}.{{ $tableName | qi}}.{{ .Name | qi }}%TYPE := NEW.{{ .Name | qi }};
      {{ end -}}
      latest_schema text;
      search_path text;
    BEGIN
      SELECT {{ .SchemaName | ql }} || '_' || latest_version
        INTO latest_schema
        FROM {{ .StateSchema | qi }}.latest_version({{ .SchemaName | ql }});

      SELECT current_setting
        INTO search_path
        FROM current_setting('search_path');

      IF search_path {{- if eq .Direction "up" }} != {{- else }} = {{ end }} latest_schema {{ if .TestExpr  -}} AND {{ .TestExpr }} {{ end -}} THEN
        NEW.{{ .PhysicalColumn | qi  }} = {{ .SQL }};
      {{- if .ElseExpr }}
      ELSE
        {{ .ElseExpr }};
      {{- end }}
      END IF;

      RETURN NEW;
    END; $$
`
```

The templates are easier to read, easier to extend and result in cleaner
triggers without oddities in indentation levels or empty `ELSE` blocks
when no `ElseExpr` is provided.

This is in response to [this
comment](#63 (comment)).
andrew-farries added a commit that referenced this pull request Sep 1, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Implement the **change column type** operation. A change type migration
looks like this:

```json
{
  "name": "18_change_column_type",
  "operations": [
    {
      "change_type": {
        "table": "reviews",
        "column": "rating",
        "type": "integer",
        "up": "CAST(rating AS integer)",
        "down": "CAST(rating AS text)"
      }
    }
  ]
}
```
This migration changes the type of the `rating` column from `TEXT` to
`INTEGER`.

The implementation is very similar to the **set NOT NULL** operation
(#63):

* On `Start`:
  *  Create a new column having the new type
* Backfill the new column with values from the existing column,
converting the types using the `up` SQL.
* Create a trigger to populate the new column when values are written to
the old column, converting types with `up`.
* Create a trigger to populate the old column when values are written to
the new column, converting types with `down`.
* On `Complete`
  * Remove triggers
  * Drop the old column
  * Rename the new column to the old column name.
* On `Rollback`
  * Remove the new column and both triggers.

The migration can fail in at least 2 ways:
* The initial backfill of existing rows on `Start` fails due to the type
conversion not being possible on one or more rows. In the above example,
any existing rows with `rating` values not representable as an `INTEGER`
will cause a failure on `Start`.
* In this case, the failure is reported and the migration rolled back
(#73)
* During the rollout period, unconvertible values are written to the old
version schema. The `up` trigger will fail to convert the values and the
`INSERT`/`UPDATE` will fail.
* Some form of data quarantine needs to be implemented here, copying the
invalid rows elsewhere and blocking completion of the migration until
those rows are handled in some way).

The PR also adds example migrations to the `/examples` directory.
andrew-farries added a commit that referenced this pull request Sep 11, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
For consistency with other operations that allow `up` SQL and to allow
more flexibility, the `up` SQL on the set `NOT NULL` operation should
run on all values, not just `NULL`s.

Making the `up` SQL here behave the same as other operations will also
make it easier to combine operations as one 'alter column' operation in
future.

This was previously discussed in the comments on #63, especially
[here](#63 (comment)).
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.

None yet

2 participants