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

Two sequences for sec_permission table #2352

Open
m0nhawk opened this issue Feb 20, 2024 · 3 comments
Open

Two sequences for sec_permission table #2352

m0nhawk opened this issue Feb 20, 2024 · 3 comments
Assignees

Comments

@m0nhawk
Copy link
Contributor

m0nhawk commented Feb 20, 2024

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
Copy link
Collaborator

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.

@chrisknoll chrisknoll self-assigned this Feb 20, 2024
@pieterlukasse
Copy link
Contributor

pieterlukasse commented Jun 5, 2024

@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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants