laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.54k stars 11.02k forks source link

Postgres sequences don’t seem to be set if running queries in transaction #22683

Closed martinbean closed 6 years ago

martinbean commented 6 years ago

Description:

I’ve created an Artisan command that imports data into a Postgres database using the DB façade. These insert statements run inside a DB::transaction() call. When I then go to interact with my application (i.e. register), I get the following error:

SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique constraint "users_pkey" DETAIL: Key (id)=(1) already exists.

Steps To Reproduce:

  1. Insert a record inside a transaction:
DB::transaction(function () {
    DB::table('users')->insert([
        // values
    ]);
});
  1. Interact with application.
  2. Get above error.

I can also confirm if I connect to the Postgres database and run SELECT nextval('users_id_seq'), I get:

ERROR: currval of sequence "users_id_seq" is not yet defined in this session

mfn commented 6 years ago

Sounds like a support request to me, see the blurb:

Please use this issue tracker only for reporting Laravel bugs.

If you need support, please use the forums:

Alternatively, you may use Slack (https://larachat.co/) or Stack Overflow (http://stackoverflow.com/questions/tagged/laravel).

If you would like to propose new Laravel features, please make a pull request, or open an issue at https://github.com/laravel/internals/issues.


How does your migration/table look like? Are you using ->increments/->bigIncrements?

martinbean commented 6 years ago

Sounds like a support request to me

Using a component that then leaves my database in an unusable state without having to execute manual statements to resolve it sounds more like a bug, and thus an Issue, to me.

sisve commented 6 years ago

@martinbean Could you provide us with the migration that created the users table and the actual table structure generated from it?

martinbean commented 6 years ago

@sisve The migration is just the default one that all new Laravel applications come with:

https://github.com/laravel/laravel/blob/master/database/migrations/2014_10_12_000000_create_users_table.php

sisve commented 6 years ago

@martinbean You have at two different points now been asked several questions; and only given us the answer to the first question.

Could you provide us with the migration that created the users table and the actual table structure generated from it?

martinbean commented 6 years ago

How does your migration/table look like? Are you using ->increments/->bigIncrements?

@sisve increments. As per default migration that ships with Laravel.

Could you provide us with the migration that created the users table and the actual table structure generated from it?

@sisve Again, the default migration for creating the users table that ships with Laravel. I don’t know what you want in terms of “actual table structure”—it’s that migration ran against the version of Postgres (2.27.1) that ships with Homestead.

@sisve If there’s other questions that I’ve “missed”, feel free to point me to them, but they’re the only ones that I could see with question marks at the end.

sisve commented 6 years ago

Can you provide us with a CREATE TABLE statement that matches the database table you have? It can probably be automatically generated for you in your database tool.

You most probably have a sequence that is generating values that is already existing in your users table. This happens if you've added rows yourself without requesting ids from the sequence, like if you have specified the ids for the inserts yourself.

mfn commented 6 years ago

like if you have specified the ids for the inserts yourself

This is also what I'm suspecting here. Have been hit by this a few time accidently with pgsql 🤷‍♀️

martinbean commented 6 years ago

like if you have specified the ids for the inserts yourself.

@sisve Yeah, this sounds like the culprit, as I’m querying one table and then inserting into another, and specifying the id value when inserting those records.

What would be my resolve in this case? Is Laravel working as intended, or is this a bug? I’d expect the database’s sequence to be kept in sync with the IDs of the record I’m inserting, as I’d imagine auto-incrementing primary keys in MySQL would be kept in sync.

sisve commented 6 years ago

Postgresql only reads (and consumes) from the sequence if it has to. If you specify the id yourself the sequence is left untouched, and when you in the future attempt to insert something the sequence says "sure, use id 1" which introduces the conflict.

The solution is to reset the sequence to the next available identifier. I do not know postgresql commands to do that.

I would classify this as a user-error when working with postgresql. Mysql does indeed support what you're doing, but postgresql requires some additional magic.

martinbean commented 6 years ago

Ah, that’s a bit annoying. I’d expect the database abstraction layer to handle things like this for me.

mfn commented 6 years ago

You can reset it with something like (untested, should get you started):

SELECT SETVAL('nameofsequence', (SELECT MAX(column) FROM table));
martinbean commented 6 years ago

@mfn Yup, I know I can manually fix the sequence. I just… would prefer not to have to, and have the database abstraction layer of the framework I’m using handle this for me.

I can’t be the only person who’s ever inserted a record into a Postgres database and manually specified the primary key value.

mfn commented 6 years ago

I can’t be the only person who’s ever inserted a record into a Postgres database and manually specified the primary key value.

Never did in production.

I do use this for fixtures I'm loading for tests (lots of) using $model::insert and then calling this sequence reset code afterwards. Fixtures have to have fixed IDs due to their inter-reference.

martinbean commented 6 years ago

@mfn Yeah, I’m in a similar situation where I’m importing data from a legacy system, so need to manually specify primary keys to maintain relations.

themsaid commented 6 years ago

Feel free to open a PR with the suggested workaround from @mfn implemented in the insert sequence or use laravel/internals for more discussion about it.

phfoxer commented 6 years ago

U can use the script below to fix all tables Copy log, remove NOTICE messages and execute in your SQL editor

DO $$ DECLARE rec RECORD; LAST_ID integer; BEGIN FOR rec IN SELECT table_name FROM information_schema.tables WHERE table_schema='public' LOOP execute 'SELECT (id + 1) as id FROM ' || rec.table_name || ' ORDER BY id DESC LIMIT 1' into LAST_ID; RAISE NOTICE 'ALTER SEQUENCE public.%_id_seq INCREMENT 1 MINVALUE 1 MAXVALUE 9223372036854775807 START 1 RESTART % CACHE 1 NO CYCLE;', rec.table_name, LAST_ID; END LOOP; END; $$

lastlink commented 6 years ago

New & improved @phfoxer.

DO $$
DECLARE
rec RECORD;
LAST_ID integer;
BEGIN
FOR rec IN SELECT
table_name
FROM information_schema.columns WHERE table_schema='public' and column_name='id' and data_type='integer'
LOOP
execute 'SELECT (id + 1) as id FROM ' || rec.table_name || ' ORDER BY id DESC LIMIT 1' into LAST_ID;
RAISE NOTICE 'ALTER SEQUENCE public.%_id_seq INCREMENT 1 MINVALUE 1 MAXVALUE 9223372036854775807 START 1 RESTART % CACHE 1 NO CYCLE;', rec.table_name, LAST_ID;
END LOOP;
END; $$
josephMG commented 5 years ago

I improve the following code to avoid empty table results in LAST_ID <NULL>,

RAISE NOTICE 'ALTER SEQUENCE public.%_id_seq INCREMENT 1 MINVALUE 1 MAXVALUE 9223372036854775807 START 1 RESTART % CACHE 1 NO CYCLE;', rec.table_name, LAST_ID;

should be:

RAISE NOTICE 'ALTER SEQUENCE public.%_id_seq INCREMENT 1 MINVALUE 1 MAXVALUE 9223372036854775807 START 1 RESTART % CACHE 1 NO CYCLE;', rec.table_name, COALESCE(LAST_ID, 1);
v1r0x commented 5 years ago

Thanks for query! I got an error because of a conflicting table name. Adding public. to the table name in the execute statement fixed it. Replaced

execute 'SELECT (id + 1) as id FROM ' || rec.table_name || ' ORDER BY id DESC LIMIT 1' into LAST_ID;

with

execute 'SELECT (id + 1) as id FROM public.' || rec.table_name || ' ORDER BY id DESC LIMIT 1' into LAST_ID;
metrey commented 5 years ago

My side not working with RAISE NOTICE, it was not execute the alter script so I change to this:

DO $$
DECLARE
rec RECORD;
LAST_ID integer;
BEGIN
FOR rec IN SELECT
table_name
FROM information_schema.columns WHERE table_schema='public' and column_name='id' and data_type='integer'
LOOP
execute 'SELECT (id + 1) as id FROM ' || rec.table_name || ' ORDER BY id DESC LIMIT 1' into LAST_ID;
execute 'ALTER SEQUENCE public.' || rec.table_name || '_id_seq INCREMENT 1 MINVALUE 1 MAXVALUE 9223372036854775807 START 1 RESTART ' || COALESCE(LAST_ID, 1) || ' CACHE 1 NO CYCLE;';
END LOOP;
END; $$
cpl-repo commented 2 years ago

Does this issue still exist, so one would need to manually run that "fix my sequence" script? Does this issue still on latest postgres version 14.x as well?

mfn commented 2 years ago

yes and yes

However, it's not an "issue" of pgsql, it's document and expected behaviour and unlikely to change.