More improvements for PostgreSQL parse / diff #161
Conversation
The parser was storing the ON UPDATE value into the constraint's on_delete attribute, and vice versa. This caused foreign keys to always show as diffs in SQL::Translator::Diff if the on_update differed from on_delete.
Test::PostgreSQL requires the postgres server binaries to be installed
on the host, which is a steep requirement. So, eval{} both the
presence of the module and whether the module is successfully able to
create a postgres instance. If so, use that. (but only if none of
the env variables were set)
The ::Producer::PostgreSQL will take a column definition of data_type 'enum' and convert it to a postgres custom data type, and then declare the column of that type. When parsed, the column no longer shows the values of the enum and the data_type is the custom name rather than 'enum'. This new option reaches into the postgres type to find the enum values, and reverts the data_type to 'enum', like it was originally.
Now, when producing a diff between two enum columns where the list of values are known, and when the postgres version is greater than 9.001, the producer will generate an ALTER TYPE ... ADD VALUE statments if the new enum includes values that the old one did not. This is an easy case to support because it does not involve re-creating the type. The harder case of removing an enum value is not supported, yet. The output includes a SQL comment to that effect, giving the user a note that they need to perform that step on their own.
|
This looks fantastic! Thanks so much for the contribution! |
|
Do you think we should warn when a diff detects removing values from an enum instead of just dropping a comment in the SQL? |
|
I'm fine with a warning, but it should probably only print once per process? Or maybe once per Translator instance? Maybe a new setting Somewhat related, in my local wrapper object I prevent all DROP statements universally, replacing them with a SQL comment and a warning, just to make sure I never deploy a production disaster. It might be nice to make that into a standard feature that all producers are aware of. |
|
That sounds like a good idea. I've been wanting to make improvements around renaming columns, which is something that also should get a warning. |
The first commit fixes a very obvious bug that creates spurrous diffs of foreign key constraints.
The second commit makes it easier to run t/66
The third commit adds a new
deconstruct_enum_typesoption to the PostgreSQL parser so that it returns a field of{ data_type => 'enum', extra => { list => ... } }instead of{ data_type => 'my_custom_enum_type' }. This gives Diff the ability to see when the schema contains a different set of enum values than the specification (only when the option is enabled).The fourth commit improves the Producer's alter_table to generate
ALTER TYPE ... ADD VALUEwhen an enum has gained a new value. This allows it to automatically upgrade schemas to include new enum values, but I didn't implement deletion of old enum values since that is much harder.