Improved Postgresql compatibility and failover support #87

Drakula2k · 2015-06-29T21:46:52Z

Fixes #27.

As far as I can see, it has been fixed in #35, but then these changes was removed in #52 because of #50 SystemStackError and other issues.

So this PR is doing almost same the same stuff as #35, but a bit more deeply and also includes few more bugfixes (including a fix for #50).

It allows the app to start and run with any set of working/down DB servers and raises errors only on write queries when master is down and on all queries when all servers are down.

mnelson · 2015-07-02T12:12:47Z

Hey @Drakula2k, thanks for putting the time into this. Let's work on getting it green. I've done an initial review and I'm on board with the desired results. I'll do a more detailed review when it's ready to ship.

Drakula2k · 2015-07-02T12:45:38Z

Thanks, I'm working on build.

…onnection errors, specs fixes

Drakula2k · 2015-07-02T21:58:49Z

Done.

Drakula2k · 2015-07-09T22:00:50Z

Also it fixes #78 and #79

sax · 2015-08-12T19:59:36Z

Would it be helpful if I tested this in our staging/production systems? I'm hoping to continue our upgrade from very old Makara (we put the upgrade on hold a while ago due to time constraints), and it looks like we would want these fixes before pushing a new version to production.

Drakula2k · 2015-08-12T20:55:20Z

@sax I'm currently using it on production, but on a small microservice which is very simple and doesn't cover all of use cases. I'm planning to use it on a bigger project in future, so I'd appreciate any feedback.

mnelson · 2015-08-12T20:58:13Z

It would be beneficial. I just now queued up a story to apply it to our staging environment, and potentially a subset of our production environment as a test. I apologize for being so unresponsive.

sax · 2015-08-12T21:00:22Z

Awesome! I actually have time allocated this week for working on our woefully ignored infrastructure upgrades, so I hope to test this tomorrow.

sax · 2015-08-14T01:09:28Z

When I run rake db:create:all I get an error, I think related to the change of ConnectionWrapper away from being a SimpleDelegator.

The first time it call :create_database on ActiveRecord::Base.connection, the context in method_missing somehow calls back to the method defined in databases.rake, not on the connection wrapper.

This is in Rails 3.2.21.

** Invoke db:drop:all (first_time)
** Invoke db:load_config (first_time)
** Execute db:load_config
** Execute db:drop:all
** Invoke db:create:all (first_time)
** Invoke db:load_config
** Execute db:create:all
wrong number of arguments (2 for 1)
/Users/sax/.gem/ruby/2.1.6/gems/activerecord-3.2.21/lib/active_record/railties/databases.rake:84:in `create_database'
/Users/sax/.gem/ruby/2.1.6/bundler/gems/makara-3be697fa290f/lib/makara/proxy.rb:75:in `block in method_missing'
/Users/sax/.gem/ruby/2.1.6/bundler/gems/makara-3be697fa290f/lib/makara/proxy.rb:117:in `block in any_connection'
/Users/sax/.gem/ruby/2.1.6/bundler/gems/makara-3be697fa290f/lib/makara/pool.rb:96:in `block in provide'
/Users/sax/.gem/ruby/2.1.6/bundler/gems/makara-3be697fa290f/lib/active_record/connection_adapters/makara_abstract_adapter.rb:33:in `handle'
/Users/sax/.gem/ruby/2.1.6/bundler/gems/makara-3be697fa290f/lib/makara/pool.rb:95:in `provide'
/Users/sax/.gem/ruby/2.1.6/bundler/gems/makara-3be697fa290f/lib/makara/proxy.rb:116:in `any_connection'
/Users/sax/.gem/ruby/2.1.6/bundler/gems/makara-3be697fa290f/lib/makara/proxy.rb:74:in `method_missing'
/Users/sax/.gem/ruby/2.1.6/gems/activerecord-3.2.21/lib/active_record/railties/databases.rake:144:in `rescue in create_database'
/Users/sax/.gem/ruby/2.1.6/gems/activerecord-3.2.21/lib/active_record/railties/databases.rake:85:in `create_database'
/Users/sax/.gem/ruby/2.1.6/gems/activerecord-3.2.21/lib/active_record/railties/databases.rake:52:in `block (5 levels) in <top (required)>'
/Users/sax/.gem/ruby/2.1.6/gems/activerecord-3.2.21/lib/active_record/railties/databases.rake:184:in `local_database?'
/Users/sax/.gem/ruby/2.1.6/gems/activerecord-3.2.21/lib/active_record/railties/databases.rake:52:in `block (4 levels) in <top (required)>'
/Users/sax/.gem/ruby/2.1.6/gems/activerecord-3.2.21/lib/active_record/railties/databases.rake:38:in `each_value'
/Users/sax/.gem/ruby/2.1.6/gems/activerecord-3.2.21/lib/active_record/railties/databases.rake:38:in `block (3 levels) in <top (required)>'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/task.rb:240:in `call'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/task.rb:240:in `block in execute'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/task.rb:235:in `each'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/task.rb:235:in `execute'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/task.rb:179:in `block in invoke_with_call_chain'
/opt/rubies/2.1.6/lib/ruby/2.1.0/monitor.rb:211:in `mon_synchronize'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/task.rb:172:in `invoke_with_call_chain'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/task.rb:165:in `invoke'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/application.rb:150:in `invoke_task'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/application.rb:106:in `block (2 levels) in top_level'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/application.rb:106:in `each'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/application.rb:106:in `block in top_level'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/application.rb:115:in `run_with_threads'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/application.rb:100:in `top_level'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/application.rb:78:in `block in run'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/application.rb:176:in `standard_exception_handling'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/lib/rake/application.rb:75:in `run'
/Users/sax/.gem/ruby/2.1.6/gems/rake-10.4.2/bin/rake:33:in `<top (required)>'
/Users/sax/.gem/ruby/2.1.6/bin/rake:23:in `load'
/Users/sax/.gem/ruby/2.1.6/bin/rake:23:in `<main>'
Couldn't create database for {"encoding"=>"unicode", "pool"=>10, "username"=>"*****", "password"=>"********", "host"=>"127.0.0.1", "port"=>5432, "min_messages"=>"WARNING", "adapter"=>"postgresql_makara", "user"=>"*****", "makara"=>{"blacklist_duration"=>30, "sticky"=>true, "master_ttl"=>5, "connection_error_matchers"=>[/pg::error: : select/, /no more connections allowed/, /result has been cleared/, /no connection to the server/, /the database system is (starting up|shutting down)/, /reset has failed/, /connection not open/i], "rescue_connection_failures"=>true, "connections"=>[{"name"=>"master", "role"=>"master"}, {"name"=>"replica_1", "role"=>"slave", "host"=>"127.0.0.1"}, {"name"=>"replica_3", "role"=>"slave", "host"=>"127.0.0.1"}]}, "database"=>"development"}

sax · 2015-08-14T01:42:46Z

In a pry session, I find the following:

in the proxy's method_missing, the connection responds to :create_database
when I go into the connection wrapper instance and look at the source of method(:create_database), it is actually the code defined in databases.rake
every method defined in databases.rake is available in the connection wrapper

Does it make any sense why this would be happening?

You might not run into this problem in Rails 4, as all of the methods in the rake file were pushed into objects.

sax · 2015-08-14T19:24:15Z

Ok, I get what happens. In Rails 3, the methods defined in databases.rake get defined on Object. When ConnectionWrapper was a delegator, everything got passed down to the wrapped database adapter, so there were no problems. Relying entirely on method_missing means that the methods defined on Object get used in preference to the wrapped connection.

I'll provide a gist to fix this for Rails 3, but don't think it's worth fixing in Makara.

sax · 2015-08-14T19:42:06Z

Here's a way of fixing this problem in a Rails 3 app.

https://gist.github.com/sax/a730ec5e3a1a768ef162

Drakula2k · 2015-08-17T11:07:23Z

Thanks, I'll try to fix it on this week.

sax · 2015-08-25T20:11:41Z

I've done some quick testing of this in our staging infrastructure, and it appears to work well. I ran into some issues with rake db:migrate, but I suspect that that might have to do with our use of Rails 3 and PGBouncer. I'll play around with that a bit more, to see if I can nail it down.

sax · 2015-08-25T21:20:58Z

The problems with rake db:migrate turned out to be more issues with the database.rake from Rails 3. I've updated the gist above to reflect the fact that now I'm just stealing the code from Rails 4 and monkeying it into Rails 3 (at least for PostgreSQL).

Drakula2k · 2015-08-27T14:58:11Z

I found a workaround, check this out.

Drakula2k · 2015-08-27T15:14:04Z

Not sure what's up with 1.9.2 builds, and 1.8.7 doesn't support .public_send. I think 1.8.7 support should be dropped, even the Ruby team doesn't support it (along with 1.9.2 and 1.9.3).

sax · 2015-08-27T17:58:58Z

I'm not sure if this will solve the root problem, but I also don't think you necessarily need to fix it. Rails 3 will only receive security updates from here on out, and this is not a problem in Rails 4. We're only running into it because we're trying to upgrade Makara in order to upgrade out app to Rails 4.

mnelson · 2015-08-27T18:17:51Z

I'm in favor of dropping < 2.0 support.

Drakula2k · 2015-08-28T05:40:36Z

SimpleDelegator prioritizes public methods too, so this fix is not only for this case.

Drakula2k · 2015-08-28T07:18:34Z

I integrated it to a bigger 4.2 project, works pretty well.

guymaliar · 2015-12-03T09:04:22Z

@mnelson is this project still maintained?

bleonard · 2016-01-05T00:21:58Z

Hi @Drakula2k - I realize it's been quite a long time with this PR. I'm starting off the new year with a resolution of managing this project and adding a few things.

What's the current state of this code? Are you using it in production?
Thanks for helping out.

bleonard · 2016-01-05T00:24:18Z

lib/makara/connection_wrapper.rb

-        target.respond_to?(m, true) ? target.__send__(m, *args, &block) : super(m, *args, &block)
-      ensure
-        $@.delete_if {|t| %r"\A#{Regexp.quote(__FILE__)}:#{__LINE__-2}:"o =~ t} if $@
+      if _makara_connection.respond_to?(m)


It feels odd to me in this method to be using the _makara_connection method instead of the @connection variable. If i'm reading it right, you could get a different object back each time. think that's a problem?

hmmm, i guess not. it's only doing it if it's non-nil.\

sax · 2016-01-05T00:37:18Z

@bleonard we have been using code from this pull request in our production site since the end of Aug 2015. Apart from our issues described above (where we have to work around the Rails 3 database.rake) it seems to be working fine.

bleonard · 2016-01-06T00:56:47Z

Awesome. Thanks @sax

Drakula2k · 2016-01-06T07:26:05Z

I'm using it on production as well. The only issue I noticed: when already connected host is going down/non-reachable, postgres adapter gives very long timeout (30 or 60 sec) before raising a connection exception, which is not configurable and should be fixed somewhere inside adapter's C code. I already tried, but no luck yet, will try to fix it again later.

bleonard · 2016-01-06T07:28:28Z

yes, we've something like that, too. then it doesn't get blacklisted. i believe it's a problem if the timeout time there is greater than the unicorn timeout or something else upstream. it's one of the things i want to look into. my current hope it to figure out where to set it on the mysql adapter to not be so long.

bleonard · 2016-01-06T07:40:07Z

ok, i'm going to merge this and test it out on our staging / prod.

Improved Postgresql compatibility and failover support

bleonard · 2016-01-06T17:06:19Z

Following up with noted timeout issue here: #102

Improved Postgresql compatibility and failover support

da4d39b

Drakula2k added 2 commits July 3, 2015 00:37

Proper disconnect handling when all nodes are down, handle jruby pg c…

ccd2375

…onnection errors, specs fixes

Specs fix

3be697f

sax mentioned this pull request Aug 25, 2015

DTrace probes? #91

Open

Prioritize calling public methods over private/protected

be0fb23

Drop Ruby < 2.0 builds

2fbbb3a

Drakula2k added 2 commits October 27, 2015 22:04

Case sensitive regexps since we're checking downcased strings anyway

703b75d

Handle more connection exceptions

cfbfd4c

bleonard reviewed Jan 5, 2016
View reviewed changes

bleonard added a commit that referenced this pull request Jan 6, 2016

Merge pull request #87 from Drakula2k/postgresql-compatibility

0de9e05

Improved Postgresql compatibility and failover support

bleonard merged commit 0de9e05 into instacart:master Jan 6, 2016

bleonard mentioned this pull request Sep 9, 2016

postgres issuing request to master for every query #79

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved Postgresql compatibility and failover support #87

Improved Postgresql compatibility and failover support #87

Drakula2k commented Jun 29, 2015 •

edited

Loading

mnelson commented Jul 2, 2015

Drakula2k commented Jul 2, 2015

Drakula2k commented Jul 2, 2015

Drakula2k commented Jul 9, 2015

sax commented Aug 12, 2015

Drakula2k commented Aug 12, 2015 •

edited

Loading

mnelson commented Aug 12, 2015

sax commented Aug 12, 2015

sax commented Aug 14, 2015

sax commented Aug 14, 2015

sax commented Aug 14, 2015

sax commented Aug 14, 2015

Drakula2k commented Aug 17, 2015

sax commented Aug 25, 2015

sax commented Aug 25, 2015

Drakula2k commented Aug 27, 2015

Drakula2k commented Aug 27, 2015

sax commented Aug 27, 2015

mnelson commented Aug 27, 2015

Drakula2k commented Aug 28, 2015

Drakula2k commented Aug 28, 2015

guymaliar commented Dec 3, 2015

bleonard commented Jan 5, 2016

bleonard Jan 5, 2016

bleonard Jan 5, 2016

sax commented Jan 5, 2016

bleonard commented Jan 6, 2016

Drakula2k commented Jan 6, 2016 •

edited

Loading

bleonard commented Jan 6, 2016

bleonard commented Jan 6, 2016

bleonard commented Jan 6, 2016

Improved Postgresql compatibility and failover support #87

Improved Postgresql compatibility and failover support #87

Conversation

Drakula2k commented Jun 29, 2015 • edited Loading

mnelson commented Jul 2, 2015

Drakula2k commented Jul 2, 2015

Drakula2k commented Jul 2, 2015

Drakula2k commented Jul 9, 2015

sax commented Aug 12, 2015

Drakula2k commented Aug 12, 2015 • edited Loading

mnelson commented Aug 12, 2015

sax commented Aug 12, 2015

sax commented Aug 14, 2015

sax commented Aug 14, 2015

sax commented Aug 14, 2015

sax commented Aug 14, 2015

Drakula2k commented Aug 17, 2015

sax commented Aug 25, 2015

sax commented Aug 25, 2015

Drakula2k commented Aug 27, 2015

Drakula2k commented Aug 27, 2015

sax commented Aug 27, 2015

mnelson commented Aug 27, 2015

Drakula2k commented Aug 28, 2015

Drakula2k commented Aug 28, 2015

guymaliar commented Dec 3, 2015

bleonard commented Jan 5, 2016

bleonard Jan 5, 2016

Choose a reason for hiding this comment

bleonard Jan 5, 2016

Choose a reason for hiding this comment

sax commented Jan 5, 2016

bleonard commented Jan 6, 2016

Drakula2k commented Jan 6, 2016 • edited Loading

bleonard commented Jan 6, 2016

bleonard commented Jan 6, 2016

bleonard commented Jan 6, 2016

Drakula2k commented Jun 29, 2015 •

edited

Loading

Drakula2k commented Aug 12, 2015 •

edited

Loading

Drakula2k commented Jan 6, 2016 •

edited

Loading