Two sequences for sec_permission table

[m0nhawk]

I noticed something strange in the DB definitions:

In here there is a table definition (I am using Postgres):

CREATE SEQUENCE ${ohdsiSchema}.SEC_PERMISSION_SEQUENCE START WITH 1000 INCREMENT BY 1 MAXVALUE 9223372036854775807 NO CYCLE;
CREATE TABLE ${ohdsiSchema}.SEC_PERMISSION(
    ID                  INTEGER NOT NULL DEFAULT NEXTVAL('${ohdsiSchema}.SEC_PERMISSION_SEQUENCE'),
    VALUE               VARCHAR(255) NOT NULL,
    DESCRIPTION     VARCHAR(255) NULL
);

But, around the code I see a lot of INSERT statements for other sequence for this table from here, e.g.:

INSERT INTO ${ohdsiSchema}.sec_permission(id, value, description) VALUES
  (nextval('${ohdsiSchema}.sec_permission_id_seq'), 'cohortdefinition:printfriendly:cohort:post', 'Get print-friendly HTML of cohort expression');

Is there any reason there are two sequences for the same thing?

[chrisknoll]

Found the same thing! When I was going through he performance optimization PR, I was looking where permissions were created (sec_permission) and noticed that there's 2 sequences (this is found in my pgAdmin for my local env):

sec_permission_id_seq sec_permission_sequence

(These are the two that you pionted out above).

Where'd they come from? To find out, I looked at postgres migration scripts and searched for each:

sec_permission_sequence was introduced in migration V1.0.0.9__shrio_security.sql

sec_permission_id_seq was introduced in migration V1.0.11.0_data-cojhort-analysis-permission.

I'm not sure why the latter introduced a new sequence, but probably a developer error where they didn't see it.

But the sec_permission_id_seq is the one used in our JPA entity definition, so that's the only one that matters.

It's unfortunate about that because the naming convention that is used isn't the one we would want: the sequence name should be {table}_sequence (or _seq is also used).

I think the fix here is to introduce a migration that will drop the un-used sequence.

[pieterlukasse]

@chrisknoll @m0nhawk the migration to fix this could be something like:

SELECT setval('sec_permission_sequence', (select max(id)+1 from sec_permission), false);
DROP SEQUENCE ohdsi.sec_permission_seq;

[chrisknoll]

Right, but dropping sec_permission_id_seq, but also updating the Entity definition to use the sec_permission_sequence. I think those 2 steps would do it.