More faster rails dbconsole #5910

Merged
merged 1 commit into from May 6, 2012

Conversation

Projects
None yet
5 participants
Contributor

route commented Apr 20, 2012

Running rails dbconsole takes:

real 0m3.389s
user 0m2.862s
sys 0m0.518s

But I just want to connect to database and I don't want to type database name, password, etc. But it takes so long ~ 3 sec. I've removed application initialization for it and it takes now:

real 0m0.683s
user 0m0.602s
sys 0m0.073s
railties/lib/rails/commands/dbconsole.rb
- def self.start(app)
- new(app).start
+ def self.database_configuration
+ YAML::load(ERB.new(IO.read("config/database.yml")).result)
@josevalim

josevalim Apr 20, 2012

Contributor

Here is where the issue lies, you can't assume that the database configuration file is at config/database.yml.

@jeremy

jeremy Apr 20, 2012

Owner

Similarly, if there's erb in it, that expects a booted app.

Could try YAML.load(Rails.root.join('config/database.yml').read) and fall back to loading the full env if YAML parsing fails or the requested config isn't available.

Contributor

route commented Apr 20, 2012

@josevalim @jeremy I've added fallback when parsing fails. For simple database.yml without app loading the result is ~0.5s and other cases it is still about 3s on my machine.

railties/lib/rails/commands/dbconsole.rb
- def initialize(app)
- @app = app
+ def self.env
+ @@env ||= if Rails.respond_to?(:env)
@tenderlove

tenderlove Apr 20, 2012

Owner

Why is this being cached? It looks like it's called once (unless there is an exception).

Contributor

route commented Apr 21, 2012

@tenderlove you are right of course no need to cache, fixed and rebased

railties/lib/rails/commands/dbconsole.rb
- new(app).start
+ def self.database_configuration
+ YAML.load(IO.read("config/database.yml"))
+ rescue Exception
@joevandyk

joevandyk Apr 21, 2012

Contributor

In general, you don't want to rescue Exception.

@route

route Apr 21, 2012

Contributor

We can catch only Psych::SyntaxError, Errno::ENOENT am I right?

Contributor

route commented Apr 27, 2012

@josevalim Is it need? Or I can close it.

Contributor

route commented May 4, 2012

A bit fixed and rebased

Contributor

route commented May 4, 2012

I'm not sure about this PR and I haven't heard any conclusions. I'll fix tests added in one of previous commits if it will need.

Owner

jeremy commented May 4, 2012

👍

Contributor

route commented May 6, 2012

@jeremy done, could you take a look?

Owner

jeremy commented May 6, 2012

Thanks @route! Could you squash to one commit?

Contributor

route commented May 6, 2012

Sure, done

jeremy added a commit that referenced this pull request May 6, 2012

Merge pull request #5910 from route/rails-dbconsole
Fast path starts the database console without loading the full Rails env

@jeremy jeremy merged commit b41bfb0 into rails:master May 6, 2012

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