rework `disable_referential_integrity` for PostgreSQL.

[Toby Ovod-Everett & Andrey Nering & Yves Senn]

Closes #17726.
Closes #10939.

This patch makes three distinct modifications:

1. no longer fall back to disabling user triggers if system triggers can't be disabled
2. warn the user when referential integrity can't be disabled
3. restore aborted transactions when referential integrity can't be disabled

The motivation behind these changes is to make the behavior of Rails
transparent and less error-prone. To require superuser privileges is not optimal
but it's what Rails currently needs. Users who absolutely rely on disabling user triggers
can patch `disable_referential_integrity`.

We should investigate `SET CONSTRAINTS` as a possible solution which does not require
superuser privileges.

/cc @matthewd

3 files changed, 136 insertions, 9 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 51c047479f..6cdf9ebd9b 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,22 @@
+*   PostgreSQL, no longer disables user triggers if system triggers can't be
+    disabled. Disabling user triggers does not fulfill what the method promises.
+    Rails currently requires superuser privileges for this method.
+
+    If you absolutely rely on this behavior, consider patching
+    `disable_referential_integrity`.
+
+    *Yves Senn*
+
+*   Restore aborted transaction state when `disable_referential_integrity` fails
+    due to missing permissions.
+
+    *Toby Ovod-Everett*, *Yves Senn*
+
+*   PostgreSQL, print warning message if `disable_referential_integrity` fails
+    due to missing permissions.
+
+    *Andrey Nering*, *Yves Senn*
+
 *   Allow `:limit` option for MySQL bigint primary key support.
 
     Example:
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb b/activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb
index 52b307c432..c1835380f9 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb
@@ -8,20 +8,39 @@ module ActiveRecord
 
         def disable_referential_integrity # :nodoc:
           if supports_disable_referential_integrity?
+            original_exception = nil
+
             begin
-              execute(tables.collect { |name| "ALTER TABLE #{quote_table_name(name)} DISABLE TRIGGER ALL" }.join(";"))
-            rescue
-              execute(tables.collect { |name| "ALTER TABLE #{quote_table_name(name)} DISABLE TRIGGER USER" }.join(";"))
+              transaction(requires_new: true) do
+                execute(tables.collect { |name| "ALTER TABLE #{quote_table_name(name)} DISABLE TRIGGER ALL" }.join(";"))
+              end
+            rescue => e
+              original_exception = e
             end
-          end
-          yield
-        ensure
-          if supports_disable_referential_integrity?
+
+            begin
+              yield
+            rescue ActiveRecord::InvalidForeignKey => e
+              warn <<-WARNING
+WARNING: Rails was not able to disable referential integrity.
+
+This is most likely caused due to missing permissions.
+Rails needs superuser privileges to disable referential integrity.
+
+    cause: #{original_exception.try(:message)}
+
+              WARNING
+              raise e
+            end
+
             begin
-              execute(tables.collect { |name| "ALTER TABLE #{quote_table_name(name)} ENABLE TRIGGER ALL" }.join(";"))
+              transaction(require_new: true) do
+                execute(tables.collect { |name| "ALTER TABLE #{quote_table_name(name)} ENABLE TRIGGER ALL" }.join(";"))
+              end
             rescue
-              execute(tables.collect { |name| "ALTER TABLE #{quote_table_name(name)} ENABLE TRIGGER USER" }.join(";"))
             end
+          else
+            yield
           end
         end
       end
diff --git a/activerecord/test/cases/adapters/postgresql/referential_integrity_test.rb b/activerecord/test/cases/adapters/postgresql/referential_integrity_test.rb
new file mode 100644
index 0000000000..98291f1bbf
--- /dev/null
+++ b/activerecord/test/cases/adapters/postgresql/referential_integrity_test.rb
@@ -0,0 +1,89 @@
+require 'cases/helper'
+require 'support/connection_helper'
+
+class PostgreSQLReferentialIntegrityTest < ActiveRecord::TestCase
+  self.use_transactional_fixtures = false
+
+  include ConnectionHelper
+
+  module MissingSuperuserPrivileges
+    def execute(sql)
+      if sql.match(/DISABLE TRIGGER ALL/) || sql.match(/ENABLE TRIGGER ALL/)
+        super "BROKEN;" rescue nil # put transaction in broken state
+        raise ActiveRecord::StatementInvalid, 'PG::InsufficientPrivilege'
+      else
+        super
+      end
+    end
+  end
+
+  def setup
+    @connection = ActiveRecord::Base.connection
+  end
+
+  def teardown
+    reset_connection
+    if ActiveRecord::Base.connection.is_a?(MissingSuperuserPrivileges)
+      raise "MissingSuperuserPrivileges patch was not removed"
+    end
+  end
+
+  def test_should_reraise_invalid_foreign_key_exception_and_show_warning
+    @connection.extend MissingSuperuserPrivileges
+
+    warning = capture(:stderr) do
+      e = assert_raises(ActiveRecord::InvalidForeignKey) do
+        @connection.disable_referential_integrity do
+          raise ActiveRecord::InvalidForeignKey, 'Should be re-raised'
+        end
+      end
+      assert_equal 'Should be re-raised', e.message
+    end
+    assert_match (/WARNING: Rails was not able to disable referential integrity/), warning
+    assert_match (/cause: PG::InsufficientPrivilege/), warning
+  end
+
+  def test_does_not_print_warning_if_no_invalid_foreign_key_exception_was_raised
+    @connection.extend MissingSuperuserPrivileges
+
+    warning = capture(:stderr) do
+      e = assert_raises(ActiveRecord::StatementInvalid) do
+        @connection.disable_referential_integrity do
+          raise ActiveRecord::StatementInvalid, 'Should be re-raised'
+        end
+      end
+      assert_equal 'Should be re-raised', e.message
+    end
+    assert warning.blank?, "expected no warnings but got:\n#{warning}"
+  end
+
+  def test_does_not_break_transactions
+    @connection.extend MissingSuperuserPrivileges
+
+    @connection.transaction do
+      @connection.disable_referential_integrity do
+        assert_transaction_is_not_broken
+      end
+      assert_transaction_is_not_broken
+    end
+  end
+
+  def test_does_not_break_nested_transactions
+    @connection.extend MissingSuperuserPrivileges
+
+    @connection.transaction do
+      @connection.transaction(requires_new: true) do
+        @connection.disable_referential_integrity do
+          assert_transaction_is_not_broken
+        end
+      end
+      assert_transaction_is_not_broken
+    end
+  end
+
+  private
+
+  def assert_transaction_is_not_broken
+    assert_equal "1", @connection.select_value("SELECT 1")
+  end
+end