Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot create multiple PKs (mixin .increments with .primary) #385

Open
hsz opened this issue Jul 18, 2014 · 25 comments

Comments

@hsz
Copy link
Contributor

commented Jul 18, 2014

I want to create a table basing on following MySQL dump:

CREATE TABLE `my_table` (
  `cmdId` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `deviceId` char(16) NOT NULL,
  `fnNumber` int(10) unsigned DEFAULT NULL,
  `chNumber` int(10) unsigned DEFAULT NULL,
  `cmd` varchar(50) DEFAULT NULL,
  `cmdDate` datetime DEFAULT NULL,
  `delivered` tinyint(1) DEFAULT NULL,
  PRIMARY KEY (`cmdId`,`deviceId`),
  KEY `deviceId` (`deviceId`),
  CONSTRAINT `tblcommands_ibfk_1` FOREIGN KEY (`deviceId`) REFERENCES `second_table` (`deviceId`) ON DELETE NO ACTION ON UPDATE NO ACTION
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

My knex looks like:

knex.schema.createTable('my_table', function(t) {
    t.primary(['cmdId', 'deviceId']);
    t.increments('cmdId');
    t.string('deviceId', 16).notNullable().references('second_table.deviceId');
    t.integer('fnNumber').unsigned();
    t.integer('chNumber').unsigned();
    t.string('cmd96', 50);
    t.dateTime('cmdDate');
    t.boolean('delivered');
});

However there is a problem with primary keys. I get following error:

Error: Multiple primary key defined

I assume that increments method already creates PK and primary method creates another one which causes an error.

Is it possible to achieve this format of table ?

@tgriesser

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2014

No, but it probably should be. I'll look into something for this.

@hsz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2014

According to the https://github.com/tgriesser/knex/blob/master/lib/dialects/mysql/schema/column.js#L30

ColumnCompiler_MySQL.prototype.increments = 'int unsigned not null auto_increment primary key';

So, .increments() creates PK. Isn't it too strong behaviour ? I think, it will be better if it will create AUTO_INCREMENT column without PK, so:

int unsigned not null auto_increment

And to create PK, we have to call .primary():

table.increments('id').primary();

It will allow to create many PKs with AUTO_INCREMENT column.

@hsz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2014

As a workaroud I use right now:

table.primary(['cmdId', 'deviceId']);
table.integer('cmdId').notNullable();
table.string('deviceId', 16).notNullable().references('second_table.deviceId');

And in then callback:

knex.schema.raw('ALTER TABLE my_table MODIFY cmdId INT UNSIGNED AUTO_INCREMENT');
@zacronos

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2016

I have a similar issue -- I want to use .increments() without it being a primary key. In my case it is for performance reasons. I want to create the table with no indexes (which means no primary key), insert many rows into it, and then add an index on the autoincrement column. This is more performant, and in fact is recommended by the Postgres documentation (see http://www.postgresql.org/docs/9.4/static/populate.html#POPULATE-RM-INDEXES), but currently seems impossible to do with knex.

@mistyharsh

This comment has been minimized.

Copy link

commented Jan 23, 2016

I also seem to run into similar issue. I am not able to create a table that has:

knex.schema.createTable("numeric_table", function (table) {
    table.integer("integer_key").primary;

   // Both increments & bigIncrements tries to create primary key.
    table.increments("increment_key");
    table.bigIncrements("big_increment_key");
});
@danielepiccone

This comment has been minimized.

Copy link

commented Apr 12, 2016

Same issue here .increments() tries to define a primary key, even if I manually specify one

@elhigu elhigu added the bug label May 19, 2016

@melalj

This comment has been minimized.

Copy link

commented Dec 6, 2016

You can use a raw query to add an increments columns without its primary key:

exports.up = function (knex) {
  return Promise.all([
    knex.raw('alter table "my_table" add column "my_seq" bigserial'),
  ]);
};

exports.down = (knex) => {
  return Promise.all([
    knex.schema.table('my_table', (t) => {
      t.dropColumn('my_seq');
    }),
  ]);
};
@abegines

This comment has been minimized.

Copy link

commented Dec 7, 2016

Or you can create de autoincrement column with

table.specificType('myid','serial');

(only for Postgresql)

But I think is much better table.increments() will create the PK only if i manually specify that with table.increments().primary()

@abegines

This comment has been minimized.

Copy link

commented Dec 7, 2016

Why not leave
increments: 'serial'
instead of
increments: 'serial primary key'

Whats the problem with that?

@amir-s

This comment has been minimized.

Copy link

commented Dec 8, 2016

I still get the same error.
IMHO .increments shouldn't create PK.

Any news on this issue?

@zacronos

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2016

Agreed -- while it is certainly possible to work around this issue in various ways, it is rather bizarre that the API doesn't have first-class support for creating an auto-increment column without making it a primary key.

With that said, I imagine this hasn't been fixed yet though because to correct this in the most sensical way would be a breaking change, and so it would have to be part of a major version bump.

@amir-s

This comment has been minimized.

Copy link

commented Dec 8, 2016

@zacronos
The thing is if we use .primary() on a columns when creating a table, the result would be a separate query for creating the table, and another alter table query to define the primary keys.
I think in MySQL (or perhaps other DBMSs too), you can not have a auto_incremement field without being PK.
So, knex creates the table with the primary key, and then it can not just alter it to define two primary keys since the table already has one.
If we could fix this, maybe .increments still create PK but allows other columns to be PK as well all in one create table query, so we probably wouldn't need a major version bump.

Or we can add another column type for this and leave .increments as is to not to break backward compatibility.

@zacronos

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2016

@amir-s: well that explains why knex behaves as it does -- it is apparently attempting to have behavior that is equivalent across the different supported DBMSs. I don't particularly like that, but at least it doesn't seem bizarre now.

The problem with your suggestion is that by definition you can't have multiple primary keys on a table. (You can have 1 composite PK on multiple columns, but that is very different from having 2 single-column PKs.)

So if .increments creates a PK, then it is impossible to allow other columns to also be marked as PK. It would be possible to create an option that can be passed to .increments to disable the primary key behavior on Postgres and any other DBMS that supports such behavior. Perhaps that's the best solution, as it doesn't break backward compatibility and it also doesn't break cross-DBMS compatibility. I still don't like that as well as changing the default behavior on Postgres so that .increments doesn't also make it a PK, but it would be a more easily accepted PR.

If anyone wants to submit a PR along those lines, I'd argue in its favor. :-)

@amir-s

This comment has been minimized.

Copy link

commented Dec 8, 2016

@zacronos Sorry, by "multiple PKs" I meant "one PK on multiple columns".

@HendPro12

This comment has been minimized.

Copy link

commented Jan 13, 2017

In agreement with all the comments above. This behavior was unexpected when trying to create a composite primary key as well as a non-key auto-incrementing column. It would be great to see this implemented.

@davidfurlong

This comment has been minimized.

Copy link

commented Apr 13, 2017

I'm running into this issue as well, but in the case of using Postgres and using serial fields as foreign keys - the knex implementation of a Postgres serial field is increments() and this make them primary keys by default. I want to be able to build sqlite3 as well as postgres, so using a raw knex query would confine me to either or. Any workaround suggestions?

@zacronos

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2017

This thread hasn't gotten much attention in a while. I'd like to reiterate what I think is an ideal, backward-compatible solution (which can thus be part of any minor release): add a new option to .increments(), maybe primaryKey: false, which (for DBMSs where this is possible) disables the primary key part of its functionality.

This is entirely backward compatible since .increments() retains its default behavior.

@davidfurlong

This comment has been minimized.

Copy link

commented Apr 13, 2017

@zacronos Took a quick look through the code. Looks trivial. Gonna throw together a PR
https://github.com/DeedMob/knex/tree/master/src

@demisx

This comment has been minimized.

Copy link

commented Aug 24, 2018

Same issue here. Have to use raw query to avoid bigIncrements() to attempt to create another primary key.

@radichc

This comment has been minimized.

Copy link

commented Nov 29, 2018

Workaround:

table.bigincrements();
table.biginteger('other').notNullable();
table.dropPrimary();
table.primary(['id', 'other']);
@wubzz

This comment has been minimized.

Copy link
Collaborator

commented Nov 30, 2018

If I understand the previous investigation on this topic correctly then pressumably knex forces a primary key strictly because some DBs (ie MySQL) cannot create serials without it being a primary key? If such is the case, I wouldn't worry too much about backwards compatibility when it comes to Postgres.

A MySQL user would then expect it to be a primary.
A Postgres user would not. (Which is evident above)

For this reason I think it's pretty safe to made an adjustment in Postgres to not force primary key, and add it to the changelog. Not as a patch, but for next major version.

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Dec 17, 2018

For this reason I think it's pretty safe to made an adjustment in Postgres to not force primary key, and add it to the changelog. Not as a patch, but for next major version.

I know tens of people who has always written their migrations just by writing only table.bigincrements('id') without .primary() also for postgresql, because it has been always working like that.

I though there was an extra option, which should be used to tell that the primary key should not be created, but looks like it is not there. At least I didn't find anything for postgres / mysql implementations about skipping primary key creation.

This would be major backwards compatibility breaking change on migrations that I would hate to see this happening. If we want to keep up patching this old migration API we first need to add versioning support for it to prevent all the people from fixing their old migrations on every knex version (those changes in migration code is really hard to verify working exactly the same with automatic tests so it is very vulnerable part in knex's users' code).

@main-dev

This comment has been minimized.

Copy link

commented Jan 6, 2019

Any solution?

@mjurincic

This comment has been minimized.

Copy link

commented Mar 26, 2019

Any news on this?

@lokisanhitleson

This comment has been minimized.

Copy link

commented May 16, 2019

This thread hasn't gotten much attention in a while. I'd like to reiterate what I think is an ideal, backward-compatible solution (which can thus be part of any minor release): add a new option to .increments(), maybe primaryKey: false, which (for DBMSs where this is possible) disables the primary key part of its functionality.

This is entirely backward compatible since .increments() retains its default behavior.

Can we use increments like this? any update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.