cakephp / phinx

PHP Database Migrations for Everyone
https://phinx.org
MIT License
4.46k stars 889 forks source link

PostgreSQL identity column insert issue 'identity column defined as GENERATED ALWAYS' #2193

Open captain-redbeard opened 1 year ago

captain-redbeard commented 1 year ago

Issue

The following error is returned when seeding a Postgres database where the seeds have values in the identity column.

[PDOException] SQLSTATE[428C9]: <<Unknown error>>: 7 ERROR:  cannot insert a non-DEFAULT value into column "id"
DETAIL:  Column "id" is an identity column defined as GENERATED ALWAYS.
HINT:  Use OVERRIDING SYSTEM VALUE to override. in /Applications/Develop/webdev/agent-commission/vendor/robmorgan/phinx/src/Phinx/Db/Adapter/PdoAdapter.php on line 346

Versions

Phinx version: 0.13.4 Database: PostgreSQL 14.7

Cause

The reason this error is returned is because the table was created with the identity column having GENERATED ALWAYS through the migration which means that no writes are allowed to this column. Source: https://www.postgresql.org/docs/15/sql-createtable.html

Proposed Solution

Update Create Table

Create the identity column with GENERATED BY DEFAULT, this allows the user specified value to take precedence. Source: https://www.postgresql.org/docs/15/sql-createtable.html

Impacted code:

Update Insert

To maintain maximum support the INSERT command could be updated to include OVERRIDING SYSTEM VALUE.

Impacted code:

Example insert statement:

INSERT INTO my_table (id, first_name, last_name)
OVERRIDING SYSTEM VALUE
VALUES (1, 'hello', 'world');
ajibarra commented 1 year ago

Hi @captain-redbeard , so you are proposing to change default definition for identity columns to use GENERATED BY DEFAULT instead of GENERATED ALWAYS?

You can pass 'generated' option to addColumn to use the definition you want.

captain-redbeard commented 1 year ago

Thanks @ajibarra, that's correct, I am proposing changing the default definition for identity columns to use GENERATED BY DEFAULT.

Alternatively or additionally the insert statement could be updated to include the OVERRIDING SYSTEM VALUE.

If you create a migration file without manually specifying the primary key and additional generated option you will get the above error.

ajibarra commented 1 year ago

@captain-redbeard Ok it makes sense...do you think you can create a PR since you have already identified the changes required?

captain-redbeard commented 1 year ago

@ajibarra have you had a chance to review the PR?

ajibarra commented 1 year ago

@ajibarra have you had a chance to review the PR?

Hi, just checked it because I am not notified when a PR is created unless I'm mentioned.

It looks good 👍 so we just need to wait for a maintainer to review and merge it.

Thanks for your contribution!