schema_format :sql should behave like schema_format :ruby #2948

Merged
merged 1 commit into from Nov 16, 2011

Conversation

Projects
None yet
@atambo
Contributor

atambo commented Sep 8, 2011

This commit adds a db:structure:load task that is run instead of
db:schema:load when schema_format is set to :sql. This patch also removes
the prefixing of the structure.sql files to mimic the use of a single
schema.rb file. The patch originates from github issue #715.

@samwgoldman

This comment has been minimized.

Show comment
Hide comment
@samwgoldman

samwgoldman Sep 8, 2011

+1

+1

@be9

This comment has been minimized.

Show comment
Hide comment
@be9

be9 Sep 9, 2011

Contributor

+1

Contributor

be9 commented Sep 9, 2011

+1

@jweathers777

This comment has been minimized.

Show comment
Hide comment
@jweathers777

jweathers777 Sep 27, 2011

+1

+1

@tonyc

This comment has been minimized.

Show comment
Hide comment
@tonyc

tonyc Oct 7, 2011

+1 here as well, although it looks like the patch doesn't apply cleanly against master. Any chance to get a more up-to-date patch?

tonyc commented Oct 7, 2011

+1 here as well, although it looks like the patch doesn't apply cleanly against master. Any chance to get a more up-to-date patch?

@atambo

This comment has been minimized.

Show comment
Hide comment
@atambo

atambo Oct 7, 2011

Contributor

@tonyc, just updated the patch, 50c2ca7 should apply against master now.

Contributor

atambo commented Oct 7, 2011

@tonyc, just updated the patch, 50c2ca7 should apply against master now.

@tonyc

This comment has been minimized.

Show comment
Hide comment
@tonyc

tonyc Oct 7, 2011

Awesome, thanks.

Edit for a little backstory why I think this patch is useful: We currently use things like sequences/constraints/etc that the SchemaDumper can't handle. Being able to dump/load from structure.sql smooths out our process for building Rails apps against a legacy database.

tonyc commented Oct 7, 2011

Awesome, thanks.

Edit for a little backstory why I think this patch is useful: We currently use things like sequences/constraints/etc that the SchemaDumper can't handle. Being able to dump/load from structure.sql smooths out our process for building Rails apps against a legacy database.

@tonyc

This comment has been minimized.

Show comment
Hide comment
@tonyc

tonyc Oct 10, 2011

Playing around with this patch a bit, I don't think it's quite right. When I run "rake," I get errors about duplicate constraints and tables. Rails.env within the tasks is set to "development", so it's trying to pipe structure.sql into my development database for tests, which is wrong, since we're running tests. We can't just hardcode the task to use abcs['test']['database'] though, because then we lose the ability to manually run "rake db:structure:load" and have the structure sucked into the development DB.

tonyc commented Oct 10, 2011

Playing around with this patch a bit, I don't think it's quite right. When I run "rake," I get errors about duplicate constraints and tables. Rails.env within the tasks is set to "development", so it's trying to pipe structure.sql into my development database for tests, which is wrong, since we're running tests. We can't just hardcode the task to use abcs['test']['database'] though, because then we lose the ability to manually run "rake db:structure:load" and have the structure sucked into the development DB.

@tonyc

This comment has been minimized.

Show comment
Hide comment
@tonyc

tonyc Oct 11, 2011

@atambo, I've made some changes that seem to work for me so far, at least while running 'rake' . I'm going to test it with other various tasks to see if it's working correctly.

Edit: Here's a squashed changeset with mine applied atop of @atambo's:

https://github.com/tonyc/rails/commit/e40a0f369f0ed0f992727a19b178f30028c8a0ac

tonyc commented Oct 11, 2011

@atambo, I've made some changes that seem to work for me so far, at least while running 'rake' . I'm going to test it with other various tasks to see if it's working correctly.

Edit: Here's a squashed changeset with mine applied atop of @atambo's:

https://github.com/tonyc/rails/commit/e40a0f369f0ed0f992727a19b178f30028c8a0ac

@ngauthier

This comment has been minimized.

Show comment
Hide comment
@tonyc

This comment has been minimized.

Show comment
Hide comment
@tonyc

tonyc Nov 4, 2011

FWIW, a coworker and I have been using my squashed changeset in a fork of Rails for a number of weeks now without issues, and I'm fairly happy with how it's working so far.

tonyc commented Nov 4, 2011

FWIW, a coworker and I have been using my squashed changeset in a fork of Rails for a number of weeks now without issues, and I'm fairly happy with how it's working so far.

schema_format :sql should behave like schema_format :ruby
This commit adds a db:structure:load task that is run instead of
db:schema:load when schema_format is set to :sql. This patch also removes
the prefixing of the structure.sql files to mimic the use of a single
schema.rb file. The patch originates from github issue #715.
@atambo

This comment has been minimized.

Show comment
Hide comment
@atambo

atambo Nov 4, 2011

Contributor

I've cherry-picked @tonyc's changes.

Contributor

atambo commented Nov 4, 2011

I've cherry-picked @tonyc's changes.

tenderlove added a commit that referenced this pull request Nov 16, 2011

Merge pull request #2948 from atambo/master
schema_format :sql should behave like schema_format :ruby

@tenderlove tenderlove merged commit 43821bf into rails:master Nov 16, 2011

@tonyc

This comment has been minimized.

Show comment
Hide comment
@tonyc

tonyc Nov 18, 2011

Thanks! <3 <3

tonyc commented Nov 18, 2011

Thanks! <3 <3

@ngauthier

This comment has been minimized.

Show comment
Hide comment
@ngauthier

ngauthier Nov 18, 2011

OMG!

OMG!

@subelsky

This comment has been minimized.

Show comment
Hide comment
@subelsky

subelsky Nov 21, 2011

Contributor

this is awesome, will definitely save me time/pain in a constraint-heavy app I'm working on

Contributor

subelsky commented Nov 21, 2011

this is awesome, will definitely save me time/pain in a constraint-heavy app I'm working on

@nixme

This comment has been minimized.

Show comment
Hide comment
@nixme

nixme Dec 5, 2011

Will this be backported to a 3.1.x release?

nixme commented Dec 5, 2011

Will this be backported to a 3.1.x release?

@Juanmcuello

This comment has been minimized.

Show comment
Hide comment
@Juanmcuello

Juanmcuello Dec 12, 2011

Contributor

I think this commit introduces some issues.

Previously to these changes, when I executed rake db:test:clone_structure
the structure was dumped (to structure.sql) and then loaded again no matter whether
the config.active_record.schema_format was set to :sql or :ruby. That was OK
because I'm explicitly saying that I want the structure to be cloned.

With these changes, when I call now the same task, although the structure is
dumped, the loaded schema depends on the value of the
config.active_record.schema_format. So If I have it set to :ruby and then I
call rake db:test:clone_structure the loaded file is schema.rb and not the
structure.sql how it is supposed to be according to the name of the task and
its description.

The same thing happens when I call rake db:test:clone. When called,
a new schema.rb is created but the file used to load the schema depends on
config.active_record.schema_format. If I have it set to :sql, a new schema.rb
is created but the structure.sql file is used to create the database. So it is
not consistent.

I didn't create an issue/pull-request because I wanted to discuss it here first,
but in case we decide to make some changes, I think we could do something like this:

https://github.com/Juanmcuello/rails/commit/88a4aeacddf1bb051ff2115b761ed24de4bbe993

Contributor

Juanmcuello commented Dec 12, 2011

I think this commit introduces some issues.

Previously to these changes, when I executed rake db:test:clone_structure
the structure was dumped (to structure.sql) and then loaded again no matter whether
the config.active_record.schema_format was set to :sql or :ruby. That was OK
because I'm explicitly saying that I want the structure to be cloned.

With these changes, when I call now the same task, although the structure is
dumped, the loaded schema depends on the value of the
config.active_record.schema_format. So If I have it set to :ruby and then I
call rake db:test:clone_structure the loaded file is schema.rb and not the
structure.sql how it is supposed to be according to the name of the task and
its description.

The same thing happens when I call rake db:test:clone. When called,
a new schema.rb is created but the file used to load the schema depends on
config.active_record.schema_format. If I have it set to :sql, a new schema.rb
is created but the structure.sql file is used to create the database. So it is
not consistent.

I didn't create an issue/pull-request because I wanted to discuss it here first,
but in case we decide to make some changes, I think we could do something like this:

https://github.com/Juanmcuello/rails/commit/88a4aeacddf1bb051ff2115b761ed24de4bbe993

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Jan 6, 2012

Contributor

If I submitted a pull request that applied cleanly to 3.1-stable, would it be accepted there? I'd love to see these changes make it into the next 3.1 patch while we await 3.2.0 final.

Contributor

derekprior commented Jan 6, 2012

If I submitted a pull request that applied cleanly to 3.1-stable, would it be accepted there? I'd love to see these changes make it into the next 3.1 patch while we await 3.2.0 final.

@Juanmcuello

This comment has been minimized.

Show comment
Hide comment
@Juanmcuello

Juanmcuello Jan 6, 2012

Contributor

@derekprior, this was already fixed in #4036. If you want the fix to be included in next release I think you will have to create a pull request to backport it.

Contributor

Juanmcuello commented Jan 6, 2012

@derekprior, this was already fixed in #4036. If you want the fix to be included in next release I think you will have to create a pull request to backport it.

@amalagaura

This comment has been minimized.

Show comment
Hide comment
@amalagaura

amalagaura May 17, 2012

@atambo I am on 3.2.3 and this doesn't work.

This is in my application.rb: config.active_record.schema_format = :sql

This is the rake trace:

** Invoke db:setup (first_time)
** Invoke db:schema:load_if_ruby (first_time)
** Invoke db:create (first_time)
** Invoke db:load_config 
** Execute db:create
** Execute db:schema:load_if_ruby
** Invoke db:schema:load (first_time)

In the console:

ActiveRecord::Base.schema_format
=> :sql

I also confirmed that the application.rb is loaded by the rake task.

@atambo I am on 3.2.3 and this doesn't work.

This is in my application.rb: config.active_record.schema_format = :sql

This is the rake trace:

** Invoke db:setup (first_time)
** Invoke db:schema:load_if_ruby (first_time)
** Invoke db:create (first_time)
** Invoke db:load_config 
** Execute db:create
** Execute db:schema:load_if_ruby
** Invoke db:schema:load (first_time)

In the console:

ActiveRecord::Base.schema_format
=> :sql

I also confirmed that the application.rb is loaded by the rake task.

@atambo

This comment has been minimized.

Show comment
Hide comment
@atambo

atambo May 21, 2012

Contributor

@amalagaura, it looks like db:setup is broken when using schema_format :sql. For now you should be able to use db:create db:migrate db:seed db:reset db:drop etc, just not db:setup.

Contributor

atambo commented May 21, 2012

@amalagaura, it looks like db:setup is broken when using schema_format :sql. For now you should be able to use db:create db:migrate db:seed db:reset db:drop etc, just not db:setup.

else
raise "Task not supported by '#{abcs[Rails.env]["adapter"]}'"
end
if ActiveRecord::Base.connection.supports_migrations?
- File.open("#{Rails.root}/db/#{Rails.env}_structure.sql", "a") { |f| f << ActiveRecord::Base.connection.dump_schema_information }

This comment has been minimized.

@morganchristiansson

morganchristiansson Jun 20, 2012

postgres pg_dump doesn't reset it's schema so these INSERT statements will fail if the search_path has not been set to include public at end of migration

@morganchristiansson

morganchristiansson Jun 20, 2012

postgres pg_dump doesn't reset it's schema so these INSERT statements will fail if the search_path has not been set to include public at end of migration

@Juanmcuello

This comment has been minimized.

Show comment
Hide comment
@Juanmcuello

Juanmcuello Jun 20, 2012

Contributor

Not sure if it's what you mean, but I think pull request #4132 already fixed it.

See also this line in master.

Contributor

Juanmcuello commented Jun 20, 2012

Not sure if it's what you mean, but I think pull request #4132 already fixed it.

See also this line in master.

@morganchristiansson

This comment has been minimized.

Show comment
Hide comment
@morganchristiansson

morganchristiansson Jun 21, 2012

Awesome, yes that should have fixed it.

Can't wait for the next rails release for this change - as currently migrating 12 databases with parallel tests in our CI takes 75 seconds (67% of the total setup cost) and this should reduce that quite a lot.

Thanks!

Awesome, yes that should have fixed it.

Can't wait for the next rails release for this change - as currently migrating 12 databases with parallel tests in our CI takes 75 seconds (67% of the total setup cost) and this should reduce that quite a lot.

Thanks!

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