Rename default sequence when table is renamed? [AR:postgres] #6874

Merged
merged 1 commit into from Jul 10, 2012

Conversation

Projects
None yet
4 participants
@robbkidd
Contributor

robbkidd commented Jun 27, 2012

Does it make sense to rename the default primary key sequence when a table is renamed?

As it is, if I rename_table :foo, :bar, I end up with a table named "bar" whose id column is incremented by a sequence named "foo_id_seq". The names being out of sync complicates things when multiple DB users are used in production (e.g. one user for migrations and one user for app connections) and permissions need to be applied to tables and their associated sequences.

Attached is proposed crazy sauce which renames the primary key sequence if its name matches the default PK sequence created by the adapter.

@rafaelfranca

View changes

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -1208,12 +1208,19 @@ def primary_key(table)
end
# Renames a table.
+ # Also renames a table's primary key sequence if the sequence name matches the
+ # ActiveRecord default.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 27, 2012

Member

In this case you should use Active Record. We only use ActiveRecord when we refer to the ActiveRecord Ruby module.

@rafaelfranca

rafaelfranca Jun 27, 2012

Member

In this case you should use Active Record. We only use ActiveRecord when we refer to the ActiveRecord Ruby module.

+
+ rename_table :test_models, :octopi
+
+ con = ActiveRecord::Base.connection

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 27, 2012

Member

I would put this is a begin/ensure block.

begin
  con = ActiveRecord::Base.connection
  pk, seq = con.pk_and_sequence_for('octopi')
  assert_equal 'octopi_id_seq', seq
ensure
  rename_table :octopi, :test_models
end

Some command can fail and we will have an inconsistent database.

@rafaelfranca

rafaelfranca Jun 27, 2012

Member

I would put this is a begin/ensure block.

begin
  con = ActiveRecord::Base.connection
  pk, seq = con.pk_and_sequence_for('octopi')
  assert_equal 'octopi_id_seq', seq
ensure
  rename_table :octopi, :test_models
end

Some command can fail and we will have an inconsistent database.

This comment has been minimized.

@robbkidd

robbkidd Jun 27, 2012

Contributor

Certainly do not want an inconsistent database. I was following the pattern of the other tests in this file. I am happy to add an ensure to the new test method.

Possibly a DRYer solution for these tests would be a teardown method:

def teardown
  rename_table :octopi, :test_models if connection.table_exists? :octopi
  super
end

I also notice that the use of the ActiveRecord::Base.connection object is handled differently in each of the pre-existing tests. Should I not just use connection, the attr_reader defined in ActiveRecord::Migration::TestHelper?

I've got another branch with these two changes (add teardown method and just use connection) ready and tested.

@robbkidd

robbkidd Jun 27, 2012

Contributor

Certainly do not want an inconsistent database. I was following the pattern of the other tests in this file. I am happy to add an ensure to the new test method.

Possibly a DRYer solution for these tests would be a teardown method:

def teardown
  rename_table :octopi, :test_models if connection.table_exists? :octopi
  super
end

I also notice that the use of the ActiveRecord::Base.connection object is handled differently in each of the pre-existing tests. Should I not just use connection, the attr_reader defined in ActiveRecord::Migration::TestHelper?

I've got another branch with these two changes (add teardown method and just use connection) ready and tested.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 27, 2012

Member

Cool. Let's wait to merge that commits in another pull request after merge this one.

@rafaelfranca

rafaelfranca Jun 27, 2012

Member

Cool. Let's wait to merge that commits in another pull request after merge this one.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2012

Member

Seems good. I did some comments inline.

I will ask for more feedback.

@tenderlove @carlosantoniodasilva

Member

rafaelfranca commented Jun 27, 2012

Seems good. I did some comments inline.

I will ask for more feedback.

@tenderlove @carlosantoniodasilva

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2012

Member

Please squash the commits at this pull request. I'm waiting for more feedback but for me this will be merged

Member

rafaelfranca commented Jun 27, 2012

Please squash the commits at this pull request. I'm waiting for more feedback but for me this will be merged

@robbkidd

This comment has been minimized.

Show comment
Hide comment
@robbkidd

robbkidd Jun 27, 2012

Contributor

Squooshed.

Contributor

robbkidd commented Jun 27, 2012

Squooshed.

@robbkidd

This comment has been minimized.

Show comment
Hide comment
@robbkidd

robbkidd Jun 27, 2012

Contributor

Assuming the pending feedback goes well, is this something that could be backported? I imagine that I would create new PRs for this change to go into 3.2, 3.1 and 3.0.

Contributor

robbkidd commented Jun 27, 2012

Assuming the pending feedback goes well, is this something that could be backported? I imagine that I would create new PRs for this change to go into 3.2, 3.1 and 3.0.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 29, 2012

Member

I don't think that this can be backported since it is adding behavior.

Member

rafaelfranca commented Jun 29, 2012

I don't think that this can be backported since it is adding behavior.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jul 1, 2012

Member

@robbkidd as a note, 3.2 is the only thing getting non-security fixes, so even if it would be backported, you'd only need to do it for 3.2

I'm not sure if this is 'new behavior' or a bugfix, though.

Member

steveklabnik commented Jul 1, 2012

@robbkidd as a note, 3.2 is the only thing getting non-security fixes, so even if it would be backported, you'd only need to do it for 3.2

I'm not sure if this is 'new behavior' or a bugfix, though.

@ghost ghost assigned rafaelfranca Jul 1, 2012

@robbkidd

This comment has been minimized.

Show comment
Hide comment
@robbkidd

robbkidd Jul 2, 2012

Contributor

@steveklabnik I'm OK with less work.

I fall on the side that this is a bugfix, but I'm also new around here.

Contributor

robbkidd commented Jul 2, 2012

@steveklabnik I'm OK with less work.

I fall on the side that this is a bugfix, but I'm also new around here.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jul 2, 2012

Member

I figured you wouldn't mind :)

Member

steveklabnik commented Jul 2, 2012

I figured you wouldn't mind :)

#
# Example:
# rename_table('octopuses', 'octopi')
def rename_table(name, new_name)
clear_cache!
execute "ALTER TABLE #{quote_table_name(name)} RENAME TO #{quote_table_name(new_name)}"
+ pk, seq = pk_and_sequence_for(new_name)
+ if seq == "#{name}_#{pk}_seq"

This comment has been minimized.

@tenderlove

tenderlove Jul 9, 2012

Member

This test is really confusing to me. Does it actually work? Wouldn't seq contain new_name, not name?

@tenderlove

tenderlove Jul 9, 2012

Member

This test is really confusing to me. Does it actually work? Wouldn't seq contain new_name, not name?

This comment has been minimized.

@robbkidd

robbkidd Jul 9, 2012

Contributor

Nope. That's the whole point of this pull request. ALTER TABLE ... RENAME TO performed on a table in postgres does not rename any sequences that happen to be associated with that table.

For example, in the tests, the table test_models gets renamed to octopi. After the table rename, the table's ID sequencer is still named test_models_id_seq. Hence the need in postgres to rename the sequence as well.

@robbkidd

robbkidd Jul 9, 2012

Contributor

Nope. That's the whole point of this pull request. ALTER TABLE ... RENAME TO performed on a table in postgres does not rename any sequences that happen to be associated with that table.

For example, in the tests, the table test_models gets renamed to octopi. After the table rename, the table's ID sequencer is still named test_models_id_seq. Hence the need in postgres to rename the sequence as well.

This comment has been minimized.

@robbkidd

robbkidd Jul 9, 2012

Contributor

More followup, if I remove this logic from the rename_table method, but keep the new test checking that the sequence matches the new name (test-driven!), I see that the sequence for the octopi table is still named test_models_id_seq.

  1) Failure:
test_rename_table_for_postgresql_should_also_rename_default_sequence(ActiveRecord::Migration::RenameTableTest) [test/cases/migration/rename_table_test.rb:79]:
Expected: "octopi_id_seq"
  Actual: "test_models_id_seq"

State left in the database:

» psql activerecord_unittest
psql (9.1.4)
Type "help" for help.

activerecord_unittest=# \d octopi 
                                   Table "public.octopi"
 Column |          Type          |                        Modifiers                         
--------+------------------------+----------------------------------------------------------
 id     | integer                | not null default nextval('test_models_id_seq'::regclass)
 url    | character varying(255) | 
Indexes:
    "test_models_pkey" PRIMARY KEY, btree (id)

activerecord_unittest=# 
@robbkidd

robbkidd Jul 9, 2012

Contributor

More followup, if I remove this logic from the rename_table method, but keep the new test checking that the sequence matches the new name (test-driven!), I see that the sequence for the octopi table is still named test_models_id_seq.

  1) Failure:
test_rename_table_for_postgresql_should_also_rename_default_sequence(ActiveRecord::Migration::RenameTableTest) [test/cases/migration/rename_table_test.rb:79]:
Expected: "octopi_id_seq"
  Actual: "test_models_id_seq"

State left in the database:

» psql activerecord_unittest
psql (9.1.4)
Type "help" for help.

activerecord_unittest=# \d octopi 
                                   Table "public.octopi"
 Column |          Type          |                        Modifiers                         
--------+------------------------+----------------------------------------------------------
 id     | integer                | not null default nextval('test_models_id_seq'::regclass)
 url    | character varying(255) | 
Indexes:
    "test_models_pkey" PRIMARY KEY, btree (id)

activerecord_unittest=# 

This comment has been minimized.

@tenderlove

tenderlove Jul 10, 2012

Member

Ah, I got it. Thanks. Sorry, I've got a cold so my brain is running on half power. :(

@tenderlove

tenderlove Jul 10, 2012

Member

Ah, I got it. Thanks. Sorry, I've got a cold so my brain is running on half power. :(

tenderlove added a commit that referenced this pull request Jul 10, 2012

Merge pull request #6874 from robbkidd/rename_sequences_too
Rename default sequence when table is renamed? [AR:postgres]

@tenderlove tenderlove merged commit d7b8f0c into rails:master Jul 10, 2012

@robbkidd

This comment has been minimized.

Show comment
Hide comment
@robbkidd

robbkidd Jul 11, 2012

Contributor

@tenderlove Is this something I could work on a backport for 3.2?

Contributor

robbkidd commented Jul 11, 2012

@tenderlove Is this something I could work on a backport for 3.2?

@robbkidd

This comment has been minimized.

Show comment
Hide comment
@robbkidd

robbkidd Jul 11, 2012

Contributor

Went ahead and threw the back-port pull request out there for people to beat up on.

Contributor

robbkidd commented Jul 11, 2012

Went ahead and threw the back-port pull request out there for people to beat up on.

drogus added a commit that referenced this pull request Jul 11, 2012

Merge pull request #7031 from robbkidd/rename_sequences_too_backport_…
…to_3-2

Back-port #6874 to 3.2: psql adapter should rename a default pk sequence during rename_table

@robbkidd robbkidd referenced this pull request in jruby/activerecord-jdbc-adapter Jul 15, 2012

Merged

Update PG: rename sequence during table rename #202

@robbkidd robbkidd deleted the robbkidd:rename_sequences_too branch Sep 28, 2016

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