-
Notifications
You must be signed in to change notification settings - Fork 75
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
Conversation
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 ```
There was a problem hiding this 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
// TODO: This function needs to be able to duplicate a column more precisely | ||
// including constraints, indexes, defaults, etc. |
There was a problem hiding this comment.
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.
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)).
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.
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)).
Implement the operation to make an existing column
NOT NULL
.The migration looks like this:
This migration adds a
NOT NULL
constraint to thereview
column in thereviews
table.Start
:NOT VALID
NOT NULL
constraintup
SQL to replaceNULL
valuesNULLs
withup
SQL.Complete
NOT VALID
NOT NULL
constraint on the new column.NOT NULL
to the new column.NOT VALID
NOT NULL
constraintRollback