heroku / heroku-buildpack-ruby

Heroku's buildpack for Ruby applications.
MIT License
786 stars 1.87k forks source link

Allow `prepared_statements: false` in database.yml #110

Closed gregburek closed 9 years ago

gregburek commented 11 years ago

Starting with AR 3.1, prepared_statements are default on when interacting with the db. Named prepared statements disallow transaction pooling with tools such as pgbouncer. Additionally, Postgres before 9.2 may experience a performance regression with certain prepared statement parameters as only the initial query plan is used for the lifetime of a prepared statement. Some paramaters would be better served by different query plans and will suffer when run.

Rails documentation change: https://github.com/rails/rails/commit/c6663235f6ac2c9e09187c8413903211ddeb76d2

Rails open issue re pgbouncer: https://github.com/rails/rails/issues/1627

I'm a bit at a loss on how to trigger the inclusion of prepared_statements: false during the database.yml injection, otherwise a PR would be attached. Do you guys have a better idea about this?

schneems commented 11 years ago

I would have users of the buildpack be aware of the issue and set the flag in code in an initializer similar to how they can configure db pool size here:

https://devcenter.heroku.com/articles/concurrency-and-database-connections

gregburek commented 11 years ago

So, something like this?

config = Rails.application.config.database_configuration[Rails.env]
config['reaping_frequency']  = ENV['DB_REAP_FREQ'] || 10 # seconds
config['pool'] = ENV['DB_POOL'] || 5
config['prepared_statements']  = ENV['DB_PREPARED_STATEMENTS'] || false
ActiveRecord::Base.establish_connection(config)
daveyeu commented 11 years ago

Wouldn't the above need to be repeated wherever connections are made? Specifically I'm thinking Unicorn and Resque after_fork blocks.

bdotdub commented 11 years ago

You can add anything to the database.yml config if you add it to the end of the DATABASE_URL, ie:

postgres://user:pass@your.compute-1.amazonaws.com:5432/database?prepared_statements=false

This line in the Ruby buildpack walks through each of the URI params and adds them to the config file: https://github.com/heroku/heroku-buildpack-ruby/blob/master/lib/language_pack/ruby.rb#L548

daveyeu commented 11 years ago

@bdotdub Doesn't seem to work for me. I added a test endpoint to give me the database config, and it yields:

{
  :adapter             => "postgresql", 
  :username            => "hvhhnhfzwaxyzv", 
  :password            => "U3ZeMl695xlMqDJeftjaQ4JJsK", 
  :port                => 6000, 
  :database            => "d8bjp5loq05526", 
  :host                => "127.0.0.1", 
  :prepared_statements => "false"
}

It isn't casting the 'false' to a boolean. Sad face.

bdotdub commented 11 years ago

Weird :penguin:

This is what I'm getting in my env:

irb(main):022:0> ENV["DATABASE_URL"]
=> "postgres://<user>:<password>@127.0.0.1:6000/database?prepared_statements=false"

irb(main):023:0> pp Rails.application.config.database_configuration[Rails.env]
{"adapter"=>"postgresql",
 "database"=>"database",
 "username"=>"<user>",
 "password"=>"<password>",
 "host"=>"127.0.0.1",
 "port"=>6000,
 "prepared_statements"=>false}
catsby commented 11 years ago

Adding to the DATABASE_URL is not recommended. While it works, it's fragile (will be lost if you rotate credentials, promote a new database, etc).

daveyeu commented 11 years ago

@bdotdub Hrm. Maybe I got it wrong. Double-checking...

gregburek commented 11 years ago

@daveyeu FYI Your db creds are in plain text above. Roll your db creds asap: heroku pg:credentials --reset

This situation is a good example of why adding config to your DATABASE_URL is a bad idea. If the creds change, the config var will be overwritten and all additions lost.

daveyeu commented 11 years ago

@gregburek Thanks for the heads up. Rotated.

While altering DATABASE_URL is perilous, @bdotdub pointed out that updating Procfile works in this case:

web: bin/pgbouncer-stunnel.sh && DATABASE_URL=$PGBOUNCER_URI?prepared_statements=false ...

I'm still seeing inconsistencies, however:

Rails.application.config.database_configuration[Rails.env][:prepared_statements] # => false
ActiveRecord::Base.connection_pool.instance_variable_get('@config')[:prepared_statements] # => "false"

:garbage:

catsby commented 11 years ago

@daveyeu What does ActiveRecord::Base.connection_config report?

daveyeu commented 11 years ago

@catsby I had to scuttle the branch so something could get QA'd. I'll take a look later.

From what I can tell, however, it looks like ActiveRecord::Base.establish_connection (the call in Unicorn's after_fork block) uses ENV['DATABASE_URL'] by default, and parses it without any casting. Supplying a config hash directly looks like it would resolve it.

catsby commented 11 years ago

@daveyeu yes, @gregburek and I confirmed that today and I'll be updating the Dev Center article Richard mentioned to address this. The answer to your original question is "yes" then, if you're using Unicorn you'll want to do this configuration setup in the after_fork block. You'll likely not want/need the initializer in that case.

gregburek commented 11 years ago

I think I also figured this out for my pgbouncer buildpack in this commit. It isn't pretty, but since I am already rewriting DATABASE_URL on dyno launch, it allows for further appending and should work across promotes and changeovers.

Thanks to @daveyeu for the inspiration.

schneems commented 9 years ago

we now no longer over-write database.yml in Rails 4.1+ so, done!