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

Add initial DB migration for PostgreSQL #108

Merged
merged 1 commit into from
Aug 31, 2024

Conversation

easybe
Copy link
Collaborator

@easybe easybe commented Aug 30, 2024

PostgreSQL does not support AUTOINCREMENT. Instead it has the SERIAL type.

To generate the migration aerich init-db together with PostgreSQL 16.3 was used.

@easybe easybe mentioned this pull request Aug 30, 2024
@@ -0,0 +1,75 @@
from tortoise import BaseDBAsyncClient
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has to be merged with the initial migration: the very first migration has to create the aerich relation, so that aerich can start tracking the applied migrations. For Postgres, this is not the case if only the second migration creates the table.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. That is how I had it in the beginning. Will do, thanks.

@easybe easybe force-pushed the gardena/eb/migrations branch from 1963af2 to 801e9cc Compare August 30, 2024 11:26
@easybe
Copy link
Collaborator Author

easybe commented Aug 30, 2024

OK, now it works.

@tsagadar
Copy link
Collaborator

The better approach would be to update the initial migration than to delete it and to create a new one. aerich might not like having updates applied in a dev pc that no longer exist.

All dev dbs that have the initial migration applied don't need the postgres flavor.

@easybe
Copy link
Collaborator Author

easybe commented Aug 30, 2024

The better approach would be to update the initial migration than to delete it and to create a new one. aerich might not like having updates applied in a dev pc that no longer exist.

All dev dbs that have the initial migration applied don't need the postgres flavor.

I don’t understand. Depending on the DB different migration code is used. Sounds perfect to me.

@tsagadar
Copy link
Collaborator

I think we never should delete a migration file - and not change them once they are in master.

In this case changing the first migration file seem ok as it only impacts Postgres installations that have not yet been supported.

Just consider how we would add Oracle support in 12 months assuming it needs special handling as well. We could not delete the initial migration and create one with the current date - right?

@easybe
Copy link
Collaborator Author

easybe commented Aug 30, 2024

I guess we would need to modify most/all the files. I understand it is fine to change the date in the filename, the first number, the version, is what counts. But, I haven’t fully verified that. For what it’s worth, our production deployment didn’t choke on this change.

@tsagadar
Copy link
Collaborator

The aerich table stores the full file name. Not going to fully reverse engineer aerich to figure out if we play with the fire or not by renaming migration files while keeping the initial version. So ok for me.

@b-rowan
Copy link
Contributor

b-rowan commented Aug 30, 2024

Hate to be this guy after this is already ready, but is it possible we can support multiple databases in migrations by changing the tortoise config? EG here - https://tortoise.github.io/setup.html#tortoise.Tortoise.init-parameters

They seem to show that you can have default db config, as well as possibly others, and choose between them. I wonder if it would improve migrations if we added a postgres and sqlite DB in there (even if unused), because it would force aerich to generate schemas for both?

PostgreSQL does not support AUTOINCREMENT. Instead it has the SERIAL type.

To generate the migration `aerich init-db` together with PostgreSQL 16.3
was used.
@easybe easybe force-pushed the gardena/eb/migrations branch from 801e9cc to 0d1b6bd Compare August 30, 2024 17:39
@easybe
Copy link
Collaborator Author

easybe commented Aug 30, 2024

Hate to be this guy after this is already ready, but is it possible we can support multiple databases in migrations by changing the tortoise config? EG here - https://tortoise.github.io/setup.html#tortoise.Tortoise.init-parameters

They seem to show that you can have default db config, as well as possibly others, and choose between them. I wonder if it would improve migrations if we added a postgres and sqlite DB in there (even if unused), because it would force aerich to generate schemas for both?

I doubt that we can trick aerich in doing that. A quick try supports that assumption. Although aerich seems to support multiple DBs, you have to associate models to them: https://github.com/tortoise/aerich?tab=readme-ov-file#multiple-databases

@b-rowan
Copy link
Contributor

b-rowan commented Aug 30, 2024

Hate to be this guy after this is already ready, but is it possible we can support multiple databases in migrations by changing the tortoise config? EG here - https://tortoise.github.io/setup.html#tortoise.Tortoise.init-parameters
They seem to show that you can have default db config, as well as possibly others, and choose between them. I wonder if it would improve migrations if we added a postgres and sqlite DB in there (even if unused), because it would force aerich to generate schemas for both?

I doubt that we can trick aerich in doing that. A quick try supports that assumption. Although aerich seems to support multiple DBs, you have to associate models to them: https://github.com/tortoise/aerich?tab=readme-ov-file#multiple-databases

Makes sense. Mildly annoying honestly, not sure why it doesn't seem to allow you to chose DBs (or even generate for all DBs)...

@easybe
Copy link
Collaborator Author

easybe commented Aug 30, 2024

Hate to be this guy after this is already ready, but is it possible we can support multiple databases in migrations by changing the tortoise config? EG here - https://tortoise.github.io/setup.html#tortoise.Tortoise.init-parameters
They seem to show that you can have default db config, as well as possibly others, and choose between them. I wonder if it would improve migrations if we added a postgres and sqlite DB in there (even if unused), because it would force aerich to generate schemas for both?

I doubt that we can trick aerich in doing that. A quick try supports that assumption. Although aerich seems to support multiple DBs, you have to associate models to them: https://github.com/tortoise/aerich?tab=readme-ov-file#multiple-databases

Makes sense. Mildly annoying honestly, not sure why it doesn't seem to allow you to chose DBs (or even generate for all DBs)...

Well, it is an open source project, I bet they won’t be opposed to such contributions 😉

@easybe easybe merged commit 6b0fc33 into UpstreamDataInc:master Aug 31, 2024
2 checks passed
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

Successfully merging this pull request may close these issues.

3 participants