dbsrgits / sql-translator

SQL::Translator (SQLFairy)
http://sqlfairy.sourceforge.net/
82 stars 91 forks source link

More improvements for PostgreSQL parse / diff #161

Closed nrdvana closed 8 months ago

nrdvana commented 10 months ago

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_types option 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 VALUE when 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.

rabbiveesh commented 9 months ago

This looks fantastic! Thanks so much for the contribution!

rabbiveesh commented 9 months ago

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 personally always review my generated upgrades, but perhaps it would be helpful to other people?

nrdvana commented 9 months ago

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 warn_unimplemented, with values once (default), each, or 0?

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.

rabbiveesh commented 9 months ago

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. I suppose ill open a new ticket about the unimplemented warnings and merge this when I'm not on mobile