Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Postgresql auto reconnect 2 #6654

Merged
merged 3 commits into from

6 participants

@stevecj

Fixes not-quite-working PostgreSQL auto-reconnection on Connection#verify! and puts test coverage in place so that it should continue to work.

@rafaelfranca
Owner

Related with #6557.

@stevecj

Right. Based on feedback from #6557, I started again putting test coverage on the re-connect behavior that was supposed to be fixed by #6477. It turned out that was not quite working right because of an incompatible change to ...::PostgteSQLAdapter#translate_exception, and there had been no test coverage to reveal this problem.

@stevecj

Pulled master and rebased, and re-ran tests to keep the branch fresh.

Anyone going to accept this? I want to get it fixed on Edge, so I can proceed to get it fixed on the Rails versions used at the company I work for, where this will resolve an open issue that can affect our users.

@rafaelfranca
Owner

Could you squash the commits? I'll ask @tenderlove to review this.

@stevecj

Sure -- all 3 together, or just squash the warning fix?

It might be easier for me to cherry pick the commits onto other version branches if the other 2 are separate.I don't know if the change that c424ab0 fixes exists on the other branches.

@stevecj

While awaiting reply, I squashed the warning fix & force-pushed.

@stevecj

Ping.

@tenderlove tenderlove was assigned
@stevecj

Ping

@stevecj

Ping

@rafaelfranca

@stevecj Aaron is already assigned to this issue. He will take a look when he have some time.

@stevecj

OK -- thx for the update.

...ord/test/cases/adapters/postgresql/connection_test.rb
((24 lines not shown))
+ CODE
+
+ begin
+ @connection.verify!
+ new_connection_pid = @connection.query('select pg_backend_pid()')
+ ensure
+ connection_class.class_eval <<-CODE
+ alias query query_unfake
+ undef query_fake
+ CODE
+ end
+
+ assert_equal original_connection_pid, new_connection_pid, "Should have a new underlying connection pid"
+ end
+
+ # Must have with_manual_interventions set to false for this
@andmej
andmej added a note

Shouldn't this be "Must have with_manual_interventions set to true for this test to run"?

@stevecj
stevecj added a note

Yes. I'll amend that.

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

Just rebased, re-ran tests & force-pushed to keep this fresh -- since it's been sitting here for quite a while.

@stevecj

Again, rebased & re-ran tests to keep the branch alive and fresh. Any chance of a pull some day?

@carlosantoniodasilva

@stevecj thanks for keeping this up-to-date with master, we surely appreciate. @tenderlove is aware of your pull request, it has already been discussed a few times, and it'll be probably merged in sooner or later. Thanks!

...ord/test/cases/adapters/postgresql/connection_test.rb
((8 lines not shown))
+ # Fail with bad connection after next query attempt.
+ connection_class = class << @connection ; self ; end
+ connection_class.class_eval <<-CODE
+ def query_fake(*args)
+ if @called ||= false
+ @connection.stubs(:status).returns(PCconn::CONNECTION_BAD)
+ raise PGError
+ else
+ @called = true
+ @connection.unstub(:status)
+ query_unfake(*args)
+ end
+ end
+
+ alias query_unfake query
+ alias query query_fake

What's up with the spaces here? Can you check that (and the other below)?

@stevecj
stevecj added a note

I think I tried them left_aligned, and thought it didn't read well, so kinda' made centered columns instead. I grant you that's pretty non-standard though. I'll fix those if you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Steve Jorgensen added some commits
Steve Jorgensen Don't crash exception translation w/ nil result attribute.
Exception.result is nil when attempting a query after PostgreSQL
disconnect, resulting in new exception:
NoMethodError: undefined method `error_field' for nil:NilClass
0d63cda
Steve Jorgensen Simulated & actual (manual/skipped) PostgreSQL auto-reconnection tests. 6d5f4de
Steve Jorgensen Stop being silly with formatting of method aliasing. 4b1bca0
@stevecj

I just fixed the silly formatting, per Carlos' comment & rebased onto the latest master.

Anything else I can do to help move this along?

@tenderlove tenderlove merged commit 0dc356e into rails:master
@stevecj

Thanks :)

@johnnypez

Will this be backported to Rails 3.2.x?

@tenderlove
Owner

If someone is willing to do the work, yes. I think there are other commits than this pull request for the bug to be fixed on 3-2-stable though.

@stevecj

FWICS, the bug that this fixes does not actually exist on the 3.x branches. The bug that was fixed by #6477 does though, so that's the fix that needs to be backported.

The test coverage from here would be helpful in preventing further regressions of psql reconnection functionality regardless of the cause, and so might be useful to backport. Please note, however, that I badly mangled the first of those tests, and subsequently mangled my initial attempt at fixing it, which I rushed through due to my embarrassment at the initial test mangling. Therefore, also see #7292 and #7294.

I can probably be available to work this back-porting, but it could take me a week or 3 to squeeze it into my schedule.

This was referenced
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 16, 2012
  1. Don't crash exception translation w/ nil result attribute.

    Steve Jorgensen authored
    Exception.result is nil when attempting a query after PostgreSQL
    disconnect, resulting in new exception:
    NoMethodError: undefined method `error_field' for nil:NilClass
  2. Stop being silly with formatting of method aliasing.

    Steve Jorgensen authored
This page is out of date. Refresh to see the latest.
View
3  activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -474,6 +474,7 @@ def active?
def reconnect!
clear_cache!
@connection.reset
+ @open_transactions = 0
configure_connection
end
@@ -1381,7 +1382,7 @@ def postgresql_version
UNIQUE_VIOLATION = "23505"
def translate_exception(exception, message)
- case exception.result.error_field(PGresult::PG_DIAG_SQLSTATE)
+ case exception.result.try(:error_field, PGresult::PG_DIAG_SQLSTATE)
when UNIQUE_VIOLATION
RecordNotUnique.new(message, exception)
when FOREIGN_KEY_VIOLATION
View
72 activerecord/test/cases/adapters/postgresql/connection_test.rb
@@ -81,5 +81,77 @@ def test_schema_names_logs_name
assert_equal 'SCHEMA', @connection.logged[0][1]
end
+ def test_reconnection_after_simulated_disconnection_with_verify
+ assert @connection.active?
+ original_connection_pid = @connection.query('select pg_backend_pid()')
+
+ # Fail with bad connection after next query attempt.
+ connection_class = class << @connection ; self ; end
+ connection_class.class_eval <<-CODE
+ def query_fake(*args)
+ if @called ||= false
+ @connection.stubs(:status).returns(PCconn::CONNECTION_BAD)
+ raise PGError
+ else
+ @called = true
+ @connection.unstub(:status)
+ query_unfake(*args)
+ end
+ end
+
+ alias query_unfake query
+ alias query query_fake
+ CODE
+
+ begin
+ @connection.verify!
+ new_connection_pid = @connection.query('select pg_backend_pid()')
+ ensure
+ connection_class.class_eval <<-CODE
+ alias query query_unfake
+ undef query_fake
+ CODE
+ end
+
+ assert_equal original_connection_pid, new_connection_pid, "Should have a new underlying connection pid"
+ end
+
+ # Must have with_manual_interventions set to true for this
+ # test to run.
+ # When prompted, restart the PostgreSQL server with the
+ # "-m fast" option or kill the individual connection assuming
+ # you know the incantation to do that.
+ # To restart PostgreSQL 9.1 on OS X, installed via MacPorts, ...
+ # sudo su postgres -c "pg_ctl restart -D /opt/local/var/db/postgresql91/defaultdb/ -m fast"
+ def test_reconnection_after_actual_disconnection_with_verify
+ skip "with_manual_interventions is false in configuration" unless ARTest.config['with_manual_interventions']
+
+ original_connection_pid = @connection.query('select pg_backend_pid()')
+
+ # Sanity check.
+ assert @connection.active?
+
+ puts 'Kill the connection now (e.g. by restarting the PostgreSQL ' +
+ 'server with the "-m fast" option) and then press enter.'
+ $stdin.gets
+
+ @connection.verify!
+
+ assert @connection.active?
+
+ # If we get no exception here, then either we re-connected successfully, or
+ # we never actually got disconnected.
+ new_connection_pid = @connection.query('select pg_backend_pid()')
+
+ assert_not_equal original_connection_pid, new_connection_pid,
+ "umm -- looks like you didn't break the connection, because we're still " +
+ "successfully querying with the same connection pid."
+
+ # Repair all fixture connections so other tests won't break.
+ @fixture_connections.each do |c|
+ c.verify!
+ end
+ end
+
end
end
View
2  activerecord/test/config.example.yml
@@ -1,5 +1,7 @@
default_connection: <%= defined?(JRUBY_VERSION) ? 'jdbcsqlite3' : 'sqlite3' %>
+with_manual_interventions: false
+
connections:
jdbcderby:
arunit: activerecord_unittest
Something went wrong with that request. Please try again.