Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #10855 (reopened defect)

Opened 4 months ago

Last modified 3 months ago

[PATCH] Use DROP TABLE IF EXISTS on :force => true

Reported by: adamwiggins Assigned to: core
Priority: low Milestone: 2.1
Component: ActiveRecord Version: edge
Severity: minor Keywords:
Cc:

Description

create_table :force => true calls drop_table first, and if it doesn't exist suppresses errors with "rescue nil". This still shows sql errors in the logs, however, and if you use the transactional_migrations plugin, it causes the entire migration to fail in any database that supports DDL transactions, such as postgresql.

The cleaner and more correct way to handle this is "DROP TABLE IF EXISTS <tablename>". MySQL, PostgreSQL, and SQLite3 all support this syntax, not sure about others. For the purposes of this patch I've changed the Postgresql adapter only, since that's the only one of the three that supports DDL transactions, and thus is most likely to be affected. If others agree this is a good approach, perhaps it should be made part of the abstract adapter.

Attachments

drop_table_if_exists.diff (0.6 kB) - added by adamwiggins on 01/19/08 00:57:49.
Drop table if exists for postgresql adapter only.
drop_table_force_test.diff (0.6 kB) - added by adamwiggins on 01/21/08 20:35:37.
unit test "drop table if exists"
drop_table_if_exists_with_test.diff (1.7 kB) - added by kampers on 02/11/08 18:23:05.

Change History

01/19/08 00:57:49 changed by adamwiggins

  • attachment drop_table_if_exists.diff added.

Drop table if exists for postgresql adapter only.

01/19/08 01:26:50 changed by bitsweat

  • milestone changed from 2.x to 2.1.

Good idea. Could you add a unit test?

01/19/08 03:21:44 changed by bitsweat

  • status changed from new to closed.
  • resolution set to untested.

01/21/08 20:34:02 changed by adamwiggins

  • status changed from closed to reopened.
  • resolution deleted.

Done. I realized adding this that there's no test for drop_table; it seemed a little weird adding a test for a specific case when there's no test for the general case, so I first added a general test (#10884). The diff is in the context of that other patch.

01/21/08 20:35:37 changed by adamwiggins

  • attachment drop_table_force_test.diff added.

unit test "drop table if exists"

02/11/08 18:22:42 changed by kampers

  • summary changed from Use DROP TABLE IF EXISTS on :force => true to [PATCH] Use DROP TABLE IF EXISTS on :force => true.

Here's a patch which includes both the change and the test, and is updated for trunk (since some files got moved around in the AR refactoring).

I also added a comment above the test to explain what's going on.

02/11/08 18:23:05 changed by kampers

  • attachment drop_table_if_exists_with_test.diff added.