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

Handles container restarts for already initialized postgresql database #183

Closed
wants to merge 1 commit into from

Conversation

davosian
Copy link

Fixes issue 182

CREATE TABLE IF NOT EXISTS vets (
id SERIAL,
id SERIAL PRIMARY KEY,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why did you change to this primary key? I suppose the CONSTRAINT pk_vets PRIMARY KEY (id) was there in order to provide the pk_vets name to the primary key.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. This should be reverted.

@@ -1,102 +1,75 @@
-- Create tables only if they don't exist
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: we could maybe refactor and modernize the PostgreSQL script following the work from the canonical version of Spring Petclinic: https://github.com/spring-projects/spring-petclinic/blob/main/src/main/resources/db/postgres/schema.sql and https://github.com/spring-projects/spring-petclinic/blob/main/src/main/resources/db/postgres/data.sql
We should bear in mind that we support 3 DAO implementations

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being closer to the canonical version certainly makes sense to me.

);

ALTER SEQUENCE visits_id_seq RESTART WITH 100;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: I'm not aware of the potential side effects of removing the sequence reset. Did you?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further testing, I found that the code currently submitted with all the changes does not work correctly: creating new objects fails. I did not check yet whether this is related to the primary key changes or if it has to do with the sequence reset. The error I am getting is of type "primary key exists" when inserting into the database.

Considering a major rework by aligning with the canonical version probably makes the most sense. I was also wondering whether the same issues exist for using the hql and mysql versions or whether this is specific to postgresql.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @davosian for your tests.
Rewriting the PostgreSQL (and probably MySQL) DDL and DML scripts to be idempotent seems like a good idea. @dsyer did it on the canonical version.
Would you like to contribute to this extension?

HSQLDB and H2 are used as in-memory databases. Schema and data are re-created each time the application is started. So we don't need to rework them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @arey , unfortunately I currently do not have the capacity to move forward with more involved contributions (besides being far from a Spring expert). I'd be happy to test any improvements with my current setup, though. I mainly set up a docker compose stack with postgresql, petclinic rest and petclinic angular.

@arey arey added the bug label Dec 12, 2024
@arey
Copy link
Member

arey commented Dec 28, 2024

Close in favour of #185

@arey arey closed this Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restarting with external database configured re-runs the database init scripts
2 participants