Add support for PostgreSQL-style ALTER COLUMN syntax#276
Conversation
PostgreSQL (and standard SQL) uses `ALTER COLUMN` instead of MySQL's `CHANGE COLUMN` / `MODIFY COLUMN` for modifying existing columns. This adds parsing and schema evaluation for the five main sub-commands: - `ALTER COLUMN col TYPE newtype` - `ALTER COLUMN col SET NOT NULL` - `ALTER COLUMN col DROP NOT NULL` - `ALTER COLUMN col SET DEFAULT expr` - `ALTER COLUMN col DROP DEFAULT` These can be combined with other actions in a single `ALTER TABLE` statement, eg.: ``` ALTER TABLE t DROP COLUMN old_col, ALTER COLUMN col TYPE SMALLINT, ALTER COLUMN col SET NOT NULL; ``` Migration generation (inverse actions + SQL fragments) is also supported for all five variants.
|
looks very repetitve, i believe all of these + the existing ones can be expressed as a single variant (e.g. can take old type, apply modifications, then use |
|
Yes, I was wondering about that. It felt a bit wrong to encode granular instructions into something that just updates everything at once, but since CHANGE and MODIFY are already here, then why not. I was also worried about code generation, but in retrospect I think that it isn't really a problem. Let me have a look and propose an updated version in the coming days! |
|
I thought about this some more, and I don't really see how that would be doable, at least not just as taking the old type, applying modifications, then using the |
|
my thinking is as follows: in the current PR it is more like CST (variants directly correspond to syntax), which can be useful but currently all the other parsing outputs more abstract nodes, and that would help to have less changes in tables.ml. But then I realize in Gen_migrations.sql_to_fragment we actually seem to need to preserve exact sql syntax used... Probably best option would be to support dialect flag when generating migrations and convert from AST to the specific dialect, but this increases a scope of this PR quite a bit. |
|
That sounds like a fair compromise. Indeed, I also would see something like what you describe, with an AST that is fairly generic and then a dialect flag to know how to generate SQL back. The PG-specific constructor that you propose could serve as a base for this future, generic mechanism, while keeping the current change small enough. I will make an update in those lines. |
|
Sorry, I didn't see this PR right away. And maybe I'm intruding, but |
|
What do you think of f13bbfa? A few notes:
Let me know if you would like me to change anything. I could also attempt merging |
| else sprintf "ALTER COLUMN %s DROP NOT NULL" (quote_id col_name) | ||
| ) changes.not_null; | ||
| Option.map (function | ||
| | Some _ -> sprintf "ALTER COLUMN %s SET DEFAULT (* unknown *)" (quote_id col_name) |
There was a problem hiding this comment.
Not sure about this (* unknown *) since that produces invalid code. It was inspired by the (* unsupported: unknown previous charset *) above, but at least that one replaced a whole statement? Do you prefer that we replace a whole statement here too?
| let inv_not_null = Option.map (fun _ -> | ||
| Sql.Type.is_strict entry.attr.domain | ||
| ) changes.not_null in |
There was a problem hiding this comment.
This is here trying to respect that the inverse of SET NOT NULL is only DROP NOT NULL if the column was nullable before, otherwise it's a no-op, but I'm not sure if that's considerations that we should worry about?
| alter name (fun cols -> | ||
| List.map (fun c -> | ||
| if c.attr.Sql.name = col_name then | ||
| { c with attr = { c.attr with extra = Sql.Constraints.remove WithDefault c.attr.extra } } |
There was a problem hiding this comment.
I am tempted to introduce a helper to update attr in depth, since the pattern shows up everywhere. Let me know if you want this.
3 participants
PostgreSQL (and standard SQL) uses
ALTER COLUMNinstead of MySQL'sCHANGE COLUMN/MODIFY COLUMNfor modifying existing columns. This adds parsing and schema evaluation for the five main sub-commands:ALTER COLUMN col TYPE newtypeALTER COLUMN col SET NOT NULLALTER COLUMN col DROP NOT NULLALTER COLUMN col SET DEFAULT exprALTER COLUMN col DROP DEFAULTThese can be combined with other actions in a single
ALTER TABLEstatement, eg.:Migration generation (inverse actions + SQL fragments) is also supported for all five variants.
I hope everything looks well; please let me know if you want me to change anything!