r4325@asus: jeremy | 2005-11-12 03:57:46 -0800

 PostgreSQL: correctly discover custom primary key sequences.  PostgreSQL: smarter sequence name defaults, stricter last_insert_id, warn on pk without sequence.  Base.reset_sequence_name analogous to reset_table_name (mostly useful for testing).  Base.define_attr_method allows nil values.  References #2594.


git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@2985 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

6 files changed, 103 insertions, 39 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 175e6f5bd5..70e6e8b32c 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,11 @@
 *SVN*
 
+* Base.reset_sequence_name analogous to reset_table_name (mostly useful for testing).  Base.define_attr_method allows nil values.  [Jeremy Kemper]
+
+* PostgreSQL: smarter sequence name defaults, stricter last_insert_id, warn on pk without sequence.  [Jeremy Kemper]
+
+* PostgreSQL: correctly discover custom primary key sequences.  #2594 [Blair Zajac <blair@orcaware.com>, meadow.nnick@gmail.com, Jeremy Kemper]
+
 * SQLServer: don't report limits for unsupported field types.  #2835 [Ryan Tomayko]
 
 * Include the Enumerable module in ActiveRecord::Errors.  [Rick Bradley <rick@rickbradley.com>]
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 7c89ba452a..e9d885f313 100755
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -646,9 +646,16 @@ module ActiveRecord #:nodoc:
         "type"
       end
 
-      # Default sequence_name.  Use set_sequence_name to override.
+      # Lazy-set the sequence name to the connection's default.  This method
+      # is only ever called once since set_sequence_name overrides it.
       def sequence_name
-        connection.default_sequence_name(table_name, primary_key)
+        reset_sequence_name
+      end
+
+      def reset_sequence_name
+        default = connection.default_sequence_name(table_name, primary_key)
+        set_sequence_name(default)
+        default
       end
 
       # Sets the table name to use to the given value, or (if the value
@@ -1053,12 +1060,12 @@ module ActiveRecord #:nodoc:
         def define_attr_method(name, value=nil, &block)
           sing = class << self; self; end
           sing.send :alias_method, "original_#{name}", name
-          if value
+          if block_given?
+            sing.send :define_method, name, &block
+          else
             # use eval instead of a block to work around a memory leak in dev
             # mode in fcgi
             sing.class_eval "def #{name}; #{value.to_s.inspect}; end"
-          else
-            sing.send :define_method, name, &block
           end
         end
 
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index 8c3cc69ec9..dc5dc4032e 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -102,7 +102,7 @@ module ActiveRecord
       def insert(sql, name = nil, pk = nil, id_value = nil, sequence_name = nil) #:nodoc:
         execute(sql, name)
         table = sql.split(" ", 4)[2]
-        id_value || last_insert_id(table, sequence_name)
+        id_value || last_insert_id(table, sequence_name || default_sequence_name(table, pk))
       end
 
       def query(sql, name = nil) #:nodoc:
@@ -199,23 +199,34 @@ module ActiveRecord
         @schema_search_path ||= query('SHOW search_path')[0][0]
       end
 
-      def default_sequence_name(table_name, pk = 'id')
-        "#{table_name}_#{pk}_seq"
+      def default_sequence_name(table_name, pk = nil)
+        default_pk, default_seq = pk_and_sequence_for(table_name)
+        default_seq || "#{table_name}_#{pk || default_pk}_seq"
       end
 
-      # Set the sequence to the max value of the table's pk.
-      def reset_pk_sequence!(table)
-        pk, sequence = pk_and_sequence_for(table)
-        if pk and sequence
-          select_value <<-end_sql, 'Reset sequence'
-            SELECT setval('#{sequence}', (SELECT COALESCE(MAX(#{pk})+(SELECT increment_by FROM #{sequence}), (SELECT min_value FROM #{sequence})) FROM #{table}), false)
-          end_sql
+      # Resets sequence to the max value of the table's pk if present.
+      def reset_pk_sequence!(table, pk = nil, sequence = nil)
+        unless pk and sequence
+          default_pk, default_sequence = pk_and_sequence_for(table)
+          pk ||= default_pk
+          sequence ||= default_sequence
+        end
+        if pk
+          if sequence
+            select_value <<-end_sql, 'Reset sequence'
+              SELECT setval('#{sequence}', (SELECT COALESCE(MAX(#{pk})+(SELECT increment_by FROM #{sequence}), (SELECT min_value FROM #{sequence})) FROM #{table}), false)
+            end_sql
+          else
+            @logger.warn "#{table} has primary key #{pk} with no default sequence" if @logger
+          end
         end
       end
 
       # Find a table's primary key and sequence.
       def pk_and_sequence_for(table)
-        execute(<<-end_sql, 'Find pk sequence')[0]
+        # First try looking for a sequence with a dependency on the
+        # given table's primary key.
+        result = execute(<<-end_sql, 'PK and serial sequence')[0]
           SELECT attr.attname, (name.nspname || '.' || seq.relname)
           FROM pg_class      seq,
                pg_attribute  attr,
@@ -232,11 +243,29 @@ module ActiveRecord
             AND cons.contype      = 'p'
             AND dep.refobjid      = '#{table}'::regclass
         end_sql
+
+        if result.nil? or result.empty?
+          # If that fails, try parsing the primary key's default value.
+          # Support the 7.x and 8.0 nextval('foo'::text) as well as
+          # the 8.1+ nextval('foo'::regclass).
+          # TODO: assumes sequence is in same schema as table.
+          result = execute(<<-end_sql, 'PK and custom sequence')[0]
+            SELECT attr.attname, (name.nspname || '.' || split_part(def.adsrc, '\\\'', 2))
+            FROM pg_class       t
+            JOIN pg_namespace   name ON (t.relnamespace = name.oid)
+            JOIN pg_attribute   attr ON (t.oid = attrelid)
+            JOIN pg_attrdef     def  ON (adrelid = attrelid AND adnum = attnum)
+            JOIN pg_constraint  cons ON (conrelid = adrelid AND adnum = conkey[1])
+            WHERE t.oid = '#{table}'::regclass
+              AND cons.contype = 'p'
+              AND def.adsrc ~ 'nextval\\\\(\\\'[^\\\']*\\\'::[^\\\\)]*\\\\)'
+          end_sql
+        end
+        result
       rescue
         nil
       end
 
-      
       def rename_table(name, new_name)
         execute "ALTER TABLE #{name} RENAME TO #{new_name}"
       end
@@ -281,9 +310,7 @@ module ActiveRecord
         BYTEA_COLUMN_TYPE_OID = 17
 
         def last_insert_id(table, sequence_name)
-          if sequence_name
-            @connection.exec("SELECT currval('#{sequence_name}')")[0][0].to_i
-          end
+          Integer(@connection.exec("SELECT currval('#{sequence_name}')")[0][0])
         end
 
         def select(sql, name = nil)
diff --git a/activerecord/test/base_test.rb b/activerecord/test/base_test.rb
index f64821361f..7be6441f1d 100755
--- a/activerecord/test/base_test.rb
+++ b/activerecord/test/base_test.rb
@@ -839,10 +839,10 @@ class BasicsTest < Test::Unit::TestCase
   def test_column_name_properly_quoted
     col_record = ColumnName.new
     col_record.references = 40
-    col_record.save
+    assert col_record.save
     col_record.references = 41
-    col_record.save
-    c2 = ColumnName.find(col_record.id)
+    assert col_record.save
+    assert_not_nil c2 = ColumnName.find(col_record.id)
     assert_equal(41, c2.references)
   end
 
diff --git a/activerecord/test/fixtures_test.rb b/activerecord/test/fixtures_test.rb
index 888d743078..41fc666945 100755
--- a/activerecord/test/fixtures_test.rb
+++ b/activerecord/test/fixtures_test.rb
@@ -173,27 +173,45 @@ end
 if Account.connection.respond_to?(:reset_pk_sequence!)
   class FixturesResetPkSequenceTest < Test::Unit::TestCase
     fixtures :accounts
+    fixtures :companies
 
-    def test_resets_to_min_pk
-      Account.delete_all
-      Account.connection.reset_pk_sequence!(Account.table_name)
+    def setup
+      @instances = [Account.new(:credit_limit => 50), Company.new(:name => 'RoR Consulting')]
+    end
+
+    def test_resets_to_min_pk_with_specified_pk_and_sequence
+      @instances.each do |instance|
+        model = instance.class
+        model.delete_all
+        model.connection.reset_pk_sequence!(model.table_name, model.primary_key, model.sequence_name)
 
-      one = Account.new(:credit_limit => 50)
-      one.save!
-      assert_equal 1, one.id
+        instance.save!
+        assert_equal 1, instance.id, "Sequence reset for #{model.table_name} failed."
+      end
     end
 
-    def test_create_fixtures_resets_sequences
-      # create_fixtures performs reset_pk_sequence!
-      max_id = create_fixtures('accounts').inject(0) do |max_id, (name, fixture)|
-        fixture_id = fixture['id'].to_i
-        fixture_id > max_id ? fixture_id : max_id
+    def test_resets_to_min_pk_with_default_pk_and_sequence
+      @instances.each do |instance|
+        model = instance.class
+        model.delete_all
+        model.connection.reset_pk_sequence!(model.table_name)
+
+        instance.save!
+        assert_equal 1, instance.id, "Sequence reset for #{model.table_name} failed."
       end
+    end
 
-      # Clone the last fixture to check that it gets the next greatest id.
-      another = Account.new(:credit_limit => 1200)
-      another.save!
-      assert_equal max_id + 1, another.id
+    def test_create_fixtures_resets_sequences
+      @instances.each do |instance|
+        max_id = create_fixtures(instance.class.table_name).inject(0) do |max_id, (name, fixture)|
+          fixture_id = fixture['id'].to_i
+          fixture_id > max_id ? fixture_id : max_id
+        end
+
+        # Clone the last fixture to check that it gets the next greatest id.
+        instance.save!
+        assert_equal max_id + 1, instance.id, "Sequence reset for #{instance.class.table_name} failed."
+      end
     end
   end
 end
diff --git a/activerecord/test/migration_test.rb b/activerecord/test/migration_test.rb
index 9c5c2285b7..3fa680b94a 100644
--- a/activerecord/test/migration_test.rb
+++ b/activerecord/test/migration_test.rb
@@ -362,6 +362,9 @@ if ActiveRecord::Base.connection.supports_migrations?
       ActiveRecord::Base.table_name_suffix = ""
       Reminder.reset_table_name
       assert_equal "schema_info", ActiveRecord::Migrator.schema_info_table_name
+    ensure
+      ActiveRecord::Base.table_name_prefix = ""
+      ActiveRecord::Base.table_name_suffix = ""
     end
   
     def test_proper_table_name
@@ -398,17 +401,20 @@ if ActiveRecord::Base.connection.supports_migrations?
       ActiveRecord::Base.table_name_prefix = 'prefix_'
       ActiveRecord::Base.table_name_suffix = '_suffix'
       Reminder.reset_table_name
+      Reminder.reset_sequence_name
       WeNeedReminders.up
       assert Reminder.create("content" => "hello world", "remind_at" => Time.now)
       assert_equal "hello world", Reminder.find(:first).content
 
       WeNeedReminders.down
       assert_raises(ActiveRecord::StatementInvalid) { Reminder.find(:first) }
+    ensure
       ActiveRecord::Base.table_name_prefix = ''
       ActiveRecord::Base.table_name_suffix = ''
       Reminder.reset_table_name
+      Reminder.reset_sequence_name
     end
-    
+
     def test_migrator_with_duplicates
       assert_raises(ActiveRecord::DuplicateMigrationVersionError) do
         ActiveRecord::Migrator.migrate(File.dirname(__FILE__) + '/fixtures/migrations_with_duplicate/', nil)