Make DB migrations idempotent. #1678
Conversation
| ( | ||
| id text NOT NULL, | ||
| annotations xml, | ||
| CONSTRAINT annotations_id_key UNIQUE (id) | ||
| ); | ||
|
|
||
| GRANT ALL ON TABLE public.annotations TO kemal; | ||
| GRANT ALL ON TABLE public.annotations TO current_user; |
There was a problem hiding this comment.
Using current_user so we no longer hardcode the username to kemal and will take whatever's provided by the config or environment variable.
TheFrenchGhosty
added
the
need-code-review
| psql --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" < config/sql/nonces.sql | ||
| psql --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" < config/sql/annotations.sql | ||
| psql --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" < config/sql/playlists.sql | ||
| psql --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" < config/sql/playlist_videos.sql |
There was a problem hiding this comment.
What's the benefit of removing this?
There was a problem hiding this comment.
This prevents the command from stopping if errors are encountered while processing the set of SQL statements. Some statements do not have a IF NOT EXISTS clause which means we'd try to create the object all the time and thus error - in that case I'd prefer if the script continued as-is and hope for the best.
|
This is off-topic but there are also some migrations in the code:
|
Invidious has an option called check_tables which, I think, can do migrations automatically. |
TheFrenchGhosty
added
unfinished
|
@Perflyst looking at the code, it seems like It seems like the shell scripts for migrating can be deleted though as |
|
This pull request has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely abandoned or outdated. If you think this pull request is still relevant and applicable, you just have to post a comment and it will be unmarked. |
|
bump, still relevant! |
|
I'm going to unsubscribe from notifications as I predict this close-unclose cycle every 30 days is going to go for a while so please email me or tag me in a new issue if there is any more work or input required on this! |
I'm going to try a fresh install soon (in the next 3/4 days) with those scripts. if everything works (I don't see why it wouldn't), the I'll merge your change :) edit: Sorry if it's taking a bit of time, we have a huge debt of issues/PRs to take care of, and we're slowly catching up ^^ |
SamantazFox
added
need-testing
|
fixes #1772 |
|
Hey, this MR got approved but not merged, what are we waiting for? |
|
It's currently just waiting for testing. As for why it hasn't been done yet: well, we're only an small team consisting of around 5 volunteers. And there's just far too many things to do right now, so some issues/PRs are going to be sitting for a long time before we can tackle it. |
Testing ^^ |
|
I don't get why it's marked as approved without testing then >___< I can test it a bit, as it will make me remove my dirty workaround and that would be cool. |
We restrict no one from testing a PR, if you can prove that it works we can then merge it. |
|
Just FYI, I've tested it on my side and it does "just work" (though might've since became stale as I haven't kept up with changes). Here are the test instructions:
* merge the PR
* configure Invidious to run using a DB username different from "kemal"
* on a blank DB, try run the migration scripts, make sure it all works
* re-run the migration scripts again, it should handle the existing objects gracefully instead of erroring
* on a blank DB again, run invidious with the check_tables functionality to make sure we haven't broken that
Provided the above works I would say this is safe to merge. It's not perfect (I think one of the queries doesn't have an "IF EXISTS" equivalent so that would still error out, but we're not losing any functionality as the previous code would error out to begin with).
However my initial objective was to start a discussion and move towards a more robust migration framework - while this is a slight improvement it's still not ideal so I'm keen for us to have a better solution. I suggested using something like Alembic in the initial post.
… On 19 Jul 2021, at 15:05, Émilien Devos ***@***.***> wrote:
I don't get why it's marked as approved without testing then >___<
I can test it a bit, as it will make me remove my dirty workaround and that would be cool.
We restrict no one from testing a PR, if you can prove that it works we can then merge it.
(A detailed proof not just "it works")
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
ATM I started building container with this changes and will try to check if it still works on my prod instance (THE BEST IDEA IN MY LIFE! TEST PR ON PROD!). To this moment I was using workaround with Dockerfile like: Where obviously Later I'll try to spawn clean invidious instance without any db data. |
|
Test one passed - It started without crash (version without any patching already crashed at this point AFAIR): |
"Approved" means that the code looks good (i.e that we didn't find issues during code review). It doesn't mean that the code is working. |
There is something like alembic in crystal, named micrate, but given how it works, It's not really different than what we do currently (i.e manually writing migration scripts). |
It's pretty different approach than I already have seen in context of other projects but it's understandable ;D |
|
I feel like _micrate_ might be good enough for us, in the sense that it keeps track of which migrations have already ran which is the problem we're trying to solve here.
I don't have a Crystal development environment set up at the moment (last time I tried I couldn't get it to compile on my M1 Macbook) but I would encourage anyone else to try it.
… On 19 Jul 2021, at 16:17, Samantaz Fox ***@***.***> wrote:
However my initial objective was to start a discussion and move towards a more robust migration framework - while this is a slight improvement it's still not ideal so I'm keen for us to have a better solution. I suggested using something like Alembic in the initial post.
There is something like alembic in crystal, named micrate, but given how it works, It's not really different than what we do currently (i.e manually writing migration scripts).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Clean install with legacy names also works: |
|
Custom username and db also works: Log is the same as before. |
|
@JuniorJPDJ Nice, thanks for testing. Now we have to test running the creation scripts against an exising database, and it shouldn't error out. |
|
It crashes on first failed command - role create: BTW. Do we need it anyway? What the hell is it supposed to do? |
|
When I removed -psql -v ON_ERROR_STOP=1 --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" <<-EOSQL
+psql --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" <<-EOSQL~/invidious $ ./docker/init-invidious-db.sh
ERROR: permission denied to create role
NOTICE: relation "channels" already exists, skipping
CREATE TABLE
GRANT
NOTICE: relation "channels_id_idx" already exists, skipping
CREATE INDEX
NOTICE: relation "videos" already exists, skipping
CREATE TABLE
GRANT
NOTICE: relation "id_idx" already exists, skipping
CREATE INDEX
NOTICE: relation "channel_videos" already exists, skipping
CREATE TABLE
GRANT
NOTICE: relation "channel_videos_ucid_idx" already exists, skipping
CREATE INDEX
NOTICE: relation "users" already exists, skipping
CREATE TABLE
GRANT
NOTICE: relation "email_unique_idx" already exists, skipping
CREATE INDEX
NOTICE: relation "session_ids" already exists, skipping
CREATE TABLE
GRANT
NOTICE: relation "session_ids_id_idx" already exists, skipping
CREATE INDEX
NOTICE: relation "nonces" already exists, skipping
CREATE TABLE
GRANT
NOTICE: relation "nonces_nonce_idx" already exists, skipping
CREATE INDEX
CREATE TABLE
GRANT
ERROR: type "privacy" already exists
NOTICE: relation "playlists" already exists, skipping
CREATE TABLE
GRANT
NOTICE: relation "playlist_videos" already exists, skipping
CREATE TABLE
GRANTI'm not sure if this Link to message if anyone needs it: https://matrix.to/#/!GMIiONJMKtiqFzSxwj:snopyta.org/$162627599713uHQfp:pussthecat.org?via=snopyta.org&via=matrix.org&via=libera.chat |
|
Also: It shouldn't be problem anyway as script is being run only on clean DB when postgres DB container creates it. If there's existing data it won't be run by postgres container. Would be cool to have it modified to remove |
|
Also this MR should probably also modify those scripts to be run on corresponding user too: |
I got the answer to that recently. This line adds a "postgres" user. It is only meant for retro-compatibility with older docker containers who had no
Ok, nice |
There's also https://github.com/luckyframework/avram; which is also similar to what we currently do except with |
|
Thanks @Rjevski! |
Fixes #1772
This is more of a discussion than anything else - the provided code will work and maybe should be merged, but I don't think it's a complete solution.
Currently we don't seem to have a good system for database migrations. The scripts in
config/migrate-scriptsseem to be written manually, hardcode a database name and username, expect to be ran on the local machine and don't use the SQL files inconfig/sql(so it's one extra thing that needs to be manually kept in sync). They're also not aware of the current database state, so an automated deployment pipeline can't tell whether it needs to run them (or if they were already applied) and the SQL in there doesn't seem to be idempotent which means it will fail because those tables are already there.I don't think implementing our own migration system is the way to go. If Crystal has something "standard" for it I suggest we use that, otherwise we could consider Alembic?
The reason behind this is that I'm trying (well, I've succeeded - with workarounds and hacks) to deploy Invidious on a PaaS such as Dokku. The PaaS deploy process allows you to run a "post-release" command which is where database migrations should go. Most web frameworks automatically detect which migrations are applied so it's safe to run the command on every deploy even if no migrations are pending; unfortunately Invidious does not have this. This change makes the migrations idempotent (by using
IF NOT EXISTSon the create statements) so any subsequent runs will not error out.