#1397 ✓not-applicable
Liseki

Dual primary key declaration in table statement for Postgresql migrations

Reported by Liseki | September 1st, 2010 @ 04:54 AM

In dm-do-adapter.rb:133 create_table_statement generates the table statement by joining the property
schema statements for each property and adding a primary key declaration.

However in postgres.rb:22 property_schema_statement for a serial property has 2 problems:

  1. I'm not sure where :quote_column_name comes from, legacy? This results in a schema statement
    with no property name, just "SERIAL PRIMARY KEY"

  2. If the :quote_column_name is replaced with quote_name(schema[:name]) we now have a correct
    schema statement like "id SERIAL PRIMARY KEY". However this now results in a dual primary key
    declaration when this is called from dm-do-adapter.rb:133 create_table_statement.

I fixed this by changing postgres.rb:24 from:
statement = "#{schema[:quote_column_name]} SERIAL PRIMARY KEY"
to:
statement = "#{schema[:quote_column_name] || quote_name(schema[:name])} SERIAL"

The primary key declaration is now left to create_table_statement in dm-do-adapter.

Comments and changes to this ticket

  • Liseki

    Liseki September 1st, 2010 @ 04:56 AM

    Sorry, I should have said using dm-migrations 1.0.0

  • Martin Gamsjaeger (snusnu)

    Martin Gamsjaeger (snusnu) October 29th, 2010 @ 03:53 PM

    • State changed from “new” to “unconfirmed”
    • Assigned user set to “Martin Gamsjaeger (snusnu)”
  • Martin Gamsjaeger (snusnu)

    Martin Gamsjaeger (snusnu) November 4th, 2010 @ 05:16 AM

    • State changed from “unconfirmed” to “not-applicable”

    I had a look at this and came to the following conclusion:

    lib/sql/postgres.rb is never mixed into the adapter and thus actually doesn't overwrite create_table_statement. Instead it uses it's own TableCreator#to_sql to produce a create table statement. The adapters only get extended with automigration specific functionality (if you have a look at the create_table_statement signature for example, you'll see that it expects a Model and properties). The functionality provided by lib/sql/postgres.rb is only used for classic migrations, where the SQL is to be built from migration objects and not readymade DataMapper objects.

    This means that patching lib/sql/postgres.rb like you suggested, wouldn't have the desired effect, since it actually wouldn't leave establishing the PRIMARY KEY up to the adapter and thus leave us with create table statements without any primary keys.

    I totally agree that this code should (and will) be unified, but this probably won't happen before any major rewrite of dm-migrations. Such an effort is in fact underway, but will probably take quite some more time. (If you're interested, have a look at do-schema)

    Did you actually run into any problems that your suggested patch solved? I'd be curious to hear about them!

    Marking this not applicable for now. We can always reopen it.

  • Liseki

    Liseki November 8th, 2010 @ 10:32 AM

    Thanks Martin. Let me try and recreate the scenario of the 'issue':
    I had a couple of models and I was auto-migrating and auto-upgrading just fine. Then I added
    some 'classical' migrations. In a particular instance it so happened that I had required migration_runner,
    then my schema files, then I did DataMapper.auto_migrate! This was in a spec helper file for RSpec.
    That is where the 'issue' showed up. Below is a sample stack trace and as you can see the error shows
    dual primary key declaration. The patch solved the 'issue' but I later decided I didn't really need the migrations
    and solved my problem without them. I am not deeply versed, yet, in the workings of DM but I guess part
    of the problem was my then assumption that auto_migrate! would pick up on my schema files as well as
    my models.

    ~ ERROR: syntax error at or near "PRIMARY" LINE 1: CREATE TABLE "objectives" ( SERIAL PRIMARY KEY, "description... ^ (code: 16801924, sql state: 42601, query: CREATE TABLE "objectives" ( SERIAL PRIMARY KEY, "description" TEXT NOT NULL, "created_at" TIMESTAMP NOT NULL, "updated_at" TIMESTAMP NOT NULL, "plan_id" INTEGER NOT NULL, PRIMARY KEY("id")), uri: postgres://moneval_old_test)

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/adapters/dm-do-adapter.rb:92 :in execute_non_query'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/adapters/dm-do-adapter.rb:92 :in block (2 levels) in create_model_storage'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/adapters/dm-do-adapter.rb:90 :in each'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/adapters/dm-do-adapter.rb:90 :in block in create_model_storage'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-do-adapter-1.0.0/lib/dm-do-adapter/adapter.rb:260 :in with_connection'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/adapters/dm-do-adapter.rb:85 :in create_model_storage'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/adapters/dm-postgres-adapter.rb:23 :in block in create_model_storage'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/adapters/dm-postgres-adapter.rb:58 :in without_notices'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/adapters/dm-postgres-adapter.rb:23 :in create_model_storage'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/auto_migration.rb:79 :in create_model_storage'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/auto_migration.rb:175 :in auto_migrate_up!'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/auto_migration.rb:130 :in auto_migrate!'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/auto_migration.rb:45 :in block in repository_execute'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-core-1.0.0/lib/dm-core/model/descendant_set.rb:33 :in block in each'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-core-1.0.0/lib/dm-core/model/descendant_set.rb:33 :in each'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-core-1.0.0/lib/dm-core/model/descendant_set.rb:33 :in each'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/auto_migration.rb:44 :in repository_execute'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-migrations-1.0.0/lib/dm-migrations/auto_migration.rb:22 :in auto_migrate!'

    /opt/local/lib/ruby1.9/gems/1.9.1/gems/dm-constraints-1.0.0/lib/dm-constraints/migrations.rb:11 :in auto_migrate!'

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

People watching this ticket

Tags

Pages