search

toy / dump

Rails app rake and capistrano tasks to create and restore dumps of database and assets
MIT License
89 stars 14 forks source link

handles MS SQL server table chunk #17

Closed iamdionysus closed 9 years ago

iamdionysus commented 9 years ago

Hi,

When the size of table is larger than CHUNK_SIZE_MAX = 3_000, it enters each_table_row method in TableManipulation. However, the SQL syntax there is using "LIMIT" which is not supported in MS SQL server. So I added syntax for SQL server which is using TOP based on the ActiveRecord::Base.connection_config[:adapter]

toy commented 9 years ago

Hi Gabriel, thanks for contribution! Few notes: Travis is failing: rails 2.3 doesn't have ActiveRecord::Base.connection_config, for other rails versions different sql is expected. Method name select_top_or_limit_where_primary_key doesn't tell that it will return sql and it will need to be changed if some other adapter will require different behaviour. Also method can be made private. You pass quoted_primary_key, but it is used only for constructing sql, so probably you should pass primary_key and construct quoted_primary_key inside. Would be good to have a spec.

iamdionysus commented 9 years ago

I will take a look and try to improve this. I thought about using ActiveRecord query interface but I guess it wi also require rails > x version. This was quick and dirty fix since we needed immediately for our project. It was great help and we got better work flow to improve our rails backend fast. It might take long but I would lIke to contribute to this. Thanks for the comment.

toy commented 9 years ago

ActiveRecord query interface can't be used in this gem as semi-raw data is expected everywhere.

iamdionysus commented 9 years ago

What about having options like rake dump SQL_SYNTAX='TSQL' or/and rake dump DB='sql_server' ?

toy commented 9 years ago

If adapter and syntax can be detected, then I don't see why specify them using environment variable(s). You can use ActiveRecord::Base.connection.adapter_name which is available in all rails versions.

iamdionysus commented 9 years ago

I changed to meet the requirement and Travis is OK. However, I couldn't write the test yet.

toy commented 9 years ago

Nice, looks much better. Why do you put commit messages in quotes? I can add the test. Please squash commits to a minimum amount you seem fit and force push the branch.

iamdionysus commented 9 years ago

The quote in commit message was error in my windows script. I don't really work on windows environment but there are some cases and I didn't know there was a bug in the script. Thanks for letting me know. I squashed the commits and force pushed it. Please check it out.

toy commented 9 years ago

Thanks for contribution! I've already merged and will release after travis finishes.