add_column bug for PostgreSQL

PATCH to postgresql adapter

Could you explain in more detail what behavior the patch changes? Other than adding the column with one query as opposed to adding it and then changing the default/nullability with additional queries I can't see what behavior is changed.

The patch does not contain a testcase, but there is a test "test_add_column_not_null_with_default" in "activerecord/test/cases/migration_test.rb" which seems like it tests this behavior?

Also the patch uses tabs for indentation, those should be replaced with two spaces. And it would be good if the patch was based on the rails repository root so it would be easier to apply.

Thank you,

Replying to skyeagle:

In migrations "add_column" does't update exists records with :default value if :nulls => false not set for PostgreSQL database

And that behaviour is fine for me: if option :null is *not* set to false, we allow null value. Thus, the :default option will be use only for new records, but not for existing ones.

In the case you explain, you have :default => 'something' and :null => true (allow null is the default behaviour). So we assume that you want to provide a default value (i.e., when no value is given) and also allow null values (i.e, when the given value is nil). In that case, the only way to update your existing records is to update it manually e.g. with a:

execute %{update table set column = 'default'}

Regards,

Replying to tarmo:

there is a test "test_add_column_not_null_with_default" in "activerecord/test/cases/migration_test.rb" which seems like it tests this behavior?

No, the test case described in test_add_column_not_null_with_default doesn't test this behaviour: it only tests that, *if* option :null => false, an exception is raise if you try to save a record with a null value for this column.

If you want to test the case when option :null is *not* set (the case described in the description of this ticket), i would write something following (sorry, i didn't test it, but here is the idea):

def test_add_column_with_default
  Person.connection.create_table :testings do |t|
    t.column :foo, :string
  end
  
  con = Person.connection     
  Person.connection.enable_identity_insert("testings", true) if current_adapter?(:SybaseAdapter)
  Person.connection.execute "insert into testings (id, foo) values (1, 'hello')"
  Person.connection.enable_identity_insert("testings", false) if current_adapter?(:SybaseAdapter)
  assert_nothing_raised {Person.connection.add_column :testings, :bar, :string, :default => "default" }
  
  # Existing records should "not" be updated
  row = Person.find(:first)
  assert_equal nil, row.bar
  
  # Inserting a null value for bar column is ok
  assert_nothing_raised(ActiveRecord::StatementInvalid) do
    Person.connection.execute "insert into testings (id, foo, bar) values (2, 'hello', NULL)"
  end
ensure
  Person.connection.drop_table :testings rescue nil
end

Regards,