Skip to content

Conversation

@michelgrootjans
Copy link
Contributor

Fixes issue 237 (#237) by forcing the a reload of the table_cache when renaming a column.

Why would we want to do this? Apparently, column definitions of a table are loaded once, then cached. If you want to rename an initial column, no harm is done. If you want to rename an already renamed column, or a column added at a later time, the cache doesn't know about it.

This problem is only necessary when running the migrations on SqlServer. I have not been able to reproduce it on a Sqlite or Postgres database.

I'm not sure if this is the best place/way to solve this problem, but it does the trick for me.

michelgrootjans and others added 3 commits January 14, 2013 13:19
Fixing issue rails-sqlserver#237

Not sure if this is the optimal solution, but it should work.
@metaskills
Copy link
Member

DDL schema statements are not supposed to be concerned with the cache. If they were, then you would see stuff like this littered in ever DDL method in core ActiveRecord, which it is not.

When changing the columns in a migration and you want to use the root level model with an updated columns cache, you are supposed to use ActiveRecord's reset_column_information method. I should note, that using root level models in a migration is also an anti-pattern.

@michelgrootjans
Copy link
Contributor Author

I should note, that using root level models in a migration is also an anti-pattern.

I agree completely.

But in our migrations, we do not use ActiveRecord models at all. Just the usual change_column, add_column, rename_column. That's it. Still we have this problem.

I'm going to try and reproduce the problem in a separate rails project and see where it leads me.

@metaskills
Copy link
Member

Please do, I would like to know where that leads you. Cheers.

@michelgrootjans
Copy link
Contributor Author

I reproduced the problem here: https://github.com/michelgrootjans/activerecord-sqlserver-adapter-sample

If you run rake db:migrate from an empty sqlserver database, you'll get an exception on the third migration. If you run rake db:migrate again, all is well.

To fix this issue, change the gem referenced in the Gemfile to my fork.

Don't forget to configure your config/database.yml

@metaskills
Copy link
Member

Awesome. Now, can you post a gist of the stack trace? Also, what version of SQL Server are you using?

@metaskills
Copy link
Member

I also have a hunch this is related to the #change interface. I see in ActiveRecord's code where #change causes a new Table object to be instantiated and this causes the cache to kick in, where using normal up/down would not.

I did some Googling and found this old issue on Rails issues.
rails/rails#6939

If this is the case, I would propose that this is a bug with ActiveRecord and could easily be tested there. If so, then this would make a great case for reopening that issue.

@michelgrootjans
Copy link
Contributor Author

I don't think this is an ActiveRecord issue. I have run the migrations against Sqlite and Postgres without any problems. Against our SqlServer 2008 database however, it fails. You'll find the stacktrace here: https://gist.github.com/4531801

@michelgrootjans
Copy link
Contributor Author

I just tried out if changing the migrations from change => up + down. This doesn't change the behavior I'm experiencing.

@metaskills
Copy link
Member

@michelgrootjans Thanks for all the leg work. I'm gonna merge this in then. Have you verified the tests are green? If not, I will run them before and after the merge later tonight.

@michelgrootjans
Copy link
Contributor Author

I haven't verified the tests, sorry. I don't have the proper setup for it.

metaskills added a commit that referenced this pull request Jul 7, 2013
Fixes issue 237: "No such column" when renaming some columns in the migrations
@metaskills metaskills merged commit 2ba76fe into rails-sqlserver:master Jul 7, 2013
@metaskills
Copy link
Member

Thanks, this will be in the new release later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants