Handle other pk types in PostgreSQL gracefully. #10505

Closed
wants to merge 1 commit into from

7 participants

@patricksrobertson

In #10410 it was noted that you can no longer create PK's with the
type of bigserial in PostgreSQL in 4.0.0.rc1. This is mostly
because the newer adapter is checking for column type with the
id column instead of just letting it pass through like it did
before.

Side effects:
You may just create a PK column of a type that you really don't
want to be your PK. As far as I can tell this was allowed in 3.2.X
and perhaps an exception should be raised if you try and do
something extremely dumb.

Let's try this again @tenderlove! I can guarantee that the activerecord suite works just fine with the postgresql ARCONN.

@patricksrobertson

Sorry to be a bother but can you give this a look @tenderlove? This is re-opening the revert from #10455 from a couple of weeks ago.

@sjmadsen

The original problem I reported in #10410 is still in 4.0.0.rc2. /cc @tenderlove

@sjmadsen

This problem remains in the official 4.0 release.

Has it slipped through the cracks or is it being tracked under a new issue/pull request?

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Jun 26, 2013
activerecord/test/cases/primary_keys_test.rb
@@ -216,3 +216,32 @@ def test_primaery_key_method_with_ansi_quotes
end
end
+if current_adapter?(:PostgreSQLAdapter)
+ class PrimaryKeyBigSerialTest < ActiveRecord::TestCase
+ self.use_transactional_fixtures = false
+
+ class Widget < ActiveRecord::Base
+ end
+
+ def setup
+ @con = ActiveRecord::Base.connection
@carlosantoniodasilva
Ruby on Rails member

Apparently you don't need this variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva
Ruby on Rails member

@sjmadsen It seems to have slipped, will take a look, thanks and sorry about that.

This also need a changelog entry.

@sjmadsen

No worries; I'm not trying to nag, so I hope it isn't seen that way. @patricksrobertson has this nearly solved and I figured gentle prodding was more appropriate than duplicating his work in a new PR.

@patricksrobertson
@Empact

Currently the included test case passes against rails/master with and without the fix. Does this problem still exist in master?

@sjmadsen

I just created a new Rails app using edge:

rails new rails-pr-10505 -d postgresql --edge

Then I created a simple migration that used bigserial for the id column and changed the dump format to SQL. The dump is still missing a statement to create the PK constraint (ALTER TABLE ONLY things ADD CONSTRAINT things_pkey PRIMARY KEY (id)), and the following fails in the Ruby console:

irb> t = Thing.create!
=> #<Thing id: nil>

I think the problem remains unfixed in master.

@rafaelfranca
Ruby on Rails member

So we will need to update the tests to break without the fix

@JonRowe

Did anyone get round to writing a test for this?

@patricksrobertson

GH notifications. amirite? I applied the test case to rails/master and it fails. I'm not sure about the earlier assumption.

@patricksrobertson

@rafaelfranca I've ensured that the test did in fact break on master without the codefix, then I rebased this PR against master, and added a CHANGELOG entry. I think it's probably ready for another look.

@patricksrobertson

@rafaelfranca is this something that can be snuck into the 4.1 milestone? I'd love to wrap this ticket up.

@patricksrobertson patricksrobertson Handle other pk types in PostgreSQL gracefully.
In #10410 it was noted that you can no longer create PK's with the
type of bigserial in PostgreSQL in 4.0.0.rc1. This is mostly
because the newer adapter is checking for column type with the
id column instead of just letting it pass through like it did
before.

Side effects:
You may just create a PK column of a type that you really don't
want to be your PK. As far as I can tell this was allowed in 3.2.X
and perhaps an exception should be raised if you try and do
something extremely dumb.
da9a243
@senny senny closed this in f25f533 May 12, 2014
@senny
Ruby on Rails member

Thanks @patricksrobertson . I merged a rebased and slightly updated version of the patch.

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