Make the primary key to use for the migration configurable #49
Conversation
Instead of relying on a table to have a primary key, we now accept the order_column option and put that everywhere. This kind of uncomfortable state about the migration now needs to be given to everyone and I think an appropriate place is the migration, since most objects in the system have access to it and they all need to agree on the value. This required passing the options down a couple more objects for the Migrator creating the Migration to have access, but I think thats acceptable for this increase in functionality.
| end | ||
|
|
||
| def can_use_order_column?(column) | ||
| numeric_type?(@columns[column][:type]) |
There was a problem hiding this comment.
What do you think about adding a check whether the picked column has a unique key constraint? People can easily shoot themselves in the foot otherwise.
There was a problem hiding this comment.
I think thats a great idea, however I actually do (somewhat embarassingly) have a situation where I don't have a uniqueness constraint on my order_column. I think this suggests the constraint of uniqueness is good to recommend but I don't think it is critical for LHM to run so I almost don't want to enforce it.
We don't have one for speed reasons, and because the gem managing the table does a kind of upsert style thing where it will always use an existing row for a given order_column value or just create it if it doesn't exist. That said my case is definitely special so I would be willing to add the constraint and then somehow remove it in my own code if you think it should definitely make it in.
|
Looks good! |
|
Thanks for the feedback @grobie ! |
|
ping /cc @arthurnn |
|
@airhorns do you mind squashing this in one commit, and rebase against latest master. I will give a final review and merge it in! |
|
@airhorns Any chance of this getting squashed and rebased? It'd be much appreciated :) |
|
@airhorns is there anything we can do to get this into master? |
|
@camilo do you care about this at all? |
This adds a new option to
Lhm.change_tablecalledorder_column. It allows the specification of a primary key column other thanidto use during the migration, or a to specify what column should be used in the absence of a primary key. At Shopify we're moving a whole bunch of tables over toBIGINT(20)sized IDs and some of our freakier tables don't have primary keys or have weird ones, so at least we need this.Implementation wise the kind of uncomfortable state about what column to use now needs to be given to everyone. I think an appropriate place is the migration, since most objects in the system have access to it and they all need to agree on the value. This required passing the options down a couple more objects for the Migrator creating the Migration to have access, but I think thats acceptable for this increase in functionality.
/cc @dylanahsmith @arthurnn