Upgrade Postgres to 14 #2074

hubertdeng123 · 2023-04-06T19:53:16Z

This bumps the default version of Postgres in self-hosted to 14

This needs to go in before the sentry PR to change the default version of postgres due to CI failures in sentry
getsentry/sentry#46999

.pre-commit-config.yaml

docker-compose.yml

BYK

Requesting changes for Docker Container registry change.

docker-compose.yml

BYK · 2023-04-11T13:14:09Z

install/upgrade-postgres.sh

+  # If this is Postgres 9.6 data, start upgrading it to 14.0 in a new volume
+  docker run --rm \
+    -v sentry-postgres:/var/lib/postgresql/9.6/data \
+    -v sentry-postgres-new:/var/lib/postgresql/14/data \


Does this create the sentry-postgres-new volume automatically?

Yep, and we've done this in the past

BYK · 2023-04-11T13:15:48Z

install/upgrade-postgres.sh

+  # Finally, remove the new old volume as we are all in sentry-postgres now
+  docker volume rm sentry-postgres-new


Consider making this optional in interactive mode in case there are issues with the migration.

install/upgrade-postgres.sh

BYK · 2023-04-11T13:16:58Z

install/upgrade-postgres.sh

+echo "${_group}Ensuring proper PostgreSQL version ..."
+
+if [[ -n "$(docker volume ls -q --filter name=sentry-postgres)" && "$(docker run --rm -v sentry-postgres:/db busybox cat /db/PG_VERSION 2>/dev/null)" == "9.6" ]]; then
+  docker volume rm sentry-postgres-new || true


Just saw the re-indexing thing below. You may wanna add an escape hatch here with a warning that this may take a while.

I don't like introducing an additional prompt asking for the user to continue with a warning since for people who have automated upgrade scripts it will cause those to start hanging (us and our daily upgrades 😅). It feels like adding an additional install option like we do for minimize-downtime and skip-commit-check also isn't super applicable here since this is a one time thing too.

I meant it for the interactive cases that we know about. I think we had a flag to indicate non-interactive installs. But you are right, it should be fine.

BYK · 2023-04-11T13:19:53Z

install/upgrade-postgres.sh

+  $dc up -d postgres
+
+  # Wait for postgres
+  RETRIES=5
+  until docker exec sentry-self-hosted-postgres-1 psql -U postgres -c "select 1" >/dev/null 2>&1 || [ $RETRIES -eq 0 ]; do
+    echo "Waiting for postgres server, $((RETRIES--)) remaining attempts..."
+    sleep 1
+  done


We should have a proper healthcheck defined for Postgres (if not we should add that now) and then should be able to do $dc up -w postgres instead which implies detached mode.

Note that this is a compose v2-only feature (and I may argue that we should drop support for v1 at this point?)

Seems like docker compose v1 will no longer be supported in June 2023 🤔. Would rather not drop support for v1 here in this PR and wait a bit longer to do that

We currently use pg_isready for a healthcheck which seems sufficient enough for a healthcheck for Postgres

install/upgrade-postgres.sh

stayallive · 2023-04-17T17:43:57Z

Am I reading this right that it basically duplicates the database to a new volume?

Doesn't this require double-ish disk space for the duration? This seems like a non-trivial thing to not mention if so 😅

hubertdeng123 · 2023-04-17T20:00:04Z

You're right @stayallive. I'll add this to the release notes as this should be made more clear

stayallive · 2023-04-17T20:09:02Z

Oh snap. Yeah, this is going to make the upgrade... a challenge on a few deployments. Glad I caught this before kicking it off 😬

stayallive · 2023-04-18T09:15:31Z

Just an FYI, I am manually running this script line by line and am running into some issues I want to document for other readers.

I started when this was still called onpremise and thus my Postgres containers boot up as: sentry_onpremise_postgres_1 instead of sentry-self-hosted-postgres-1 which fails to reindex, manually changing the container names works just fine of course. Not sure what would happen if I would have run the script (assuming it just skips the indexing).

Also I am running the migration on one of my modest ~200GB deployments and it takes quite a hot minute to run this migration causing multi-hour donwtime (currently projecting 3-4 hours). The upgrade copies the data over to a new volume but copying the data back to the old volume name also takes a long time and reindex is also not that quick. Most of this is a docker limitation (not being able to rename the container) but was a bit unexpected going into this.

This is all not an issue that needs to be fixed (althought the container names might be fixable by running this through docker-compose instead) but I feel like the warning should be a little harder since this will cause long downtimes and undefined behaviour if you do not have double disk space available for the temp postgres volumes on larger deployments.

BYK · 2023-04-18T15:34:44Z

I started when this was still called onpremise and thus my Postgres containers boot up as: sentry_onpremise_postgres_1 instead of sentry-self-hosted-postgres-1 which fails to reindex, manually changing the container names works just fine of course. Not sure what would happen if I would have run the script (assuming it just skips the indexing).

I think if you run docker compose down it will just remove the containers and then later recreate them and you should not have any issues. There's no data stored inside containers, only in volumes, which have consistent names that did not change. Especially for the database.

Also I am running the migration on one of my modest ~200GB deployments and it takes quite a hot minute to run this migration causing multi-hour donwtime (currently projecting 3-4 hours). The upgrade copies the data over to a new volume but copying the data back to the old volume name also takes a long time and reindex is also not that quick. Most of this is a docker limitation (not being able to rename the container) but was a bit unexpected going into this.

Although it is not practical to rename a volume consistently through docker commands, there are some hacks available to you: moby/moby#31154 (comment)

This is all not an issue that needs to be fixed (althought the container names might be fixable by running this through docker-compose instead) but I feel like the warning should be a little harder since this will cause long downtimes and undefined behaviour if you do not have double disk space available for the temp postgres volumes on larger deployments.

You may try the experimental --minimize-downtime flag on the install.sh script which keeps relay and nginx running so you at least should not lose events during the upgrade, as long as relay queue does not run out of space.

hubertdeng123 · 2023-04-18T15:57:09Z

I've put up a PR to resolve the inconsistencies between docker volume names by using docker compose exec postgres instead:
#2096

Just tested it locally and it seems to work for me. Unfortunately this won't go in until the next release but hopefully that will make the upgrade process more smooth. Thanks for bringing these issues up 🙏

hubertdeng123 added 6 commits April 4, 2023 11:47

add postgres upgrade script

e94ddca

use sentry postgres image mirror

1837d46

fix scripts

5aa751a

add newline

f4153c7

pre-commit

e63a017

remove shebang check for executables

20ef518

hubertdeng123 commented Apr 6, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

hubertdeng123 requested review from BYK and a team and removed request for BYK April 6, 2023 22:13

aldy505 reviewed Apr 10, 2023

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

hubertdeng123 added 2 commits April 10, 2023 09:30

remove executable from install-postgres script

e5aad52

add comment on the reasoning of using ghcr mirror for postgres

4ec57c4

chadwhitacre approved these changes Apr 10, 2023

View reviewed changes

BYK requested changes Apr 11, 2023

View reviewed changes

use official docker image

2a0fe96

hubertdeng123 force-pushed the hubertdeng123/upgrade-postgres-to-14 branch from aa1b50b to 2a0fe96 Compare April 11, 2023 22:16

hubertdeng123 added 2 commits April 11, 2023 15:27

remove stray space

93229c1

edit note to say using same postgres version as Sentry dev

a3ee308

hubertdeng123 merged commit a6d46a9 into master Apr 12, 2023

hubertdeng123 deleted the hubertdeng123/upgrade-postgres-to-14 branch April 12, 2023 00:15

hubertdeng123 added this to the Postgres 14 Upgrade milestone Apr 14, 2023

github-actions bot locked and limited conversation to collaborators May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Postgres to 14 #2074

Upgrade Postgres to 14 #2074

hubertdeng123 commented Apr 6, 2023 •

edited

Loading

BYK left a comment

BYK Apr 11, 2023

hubertdeng123 Apr 11, 2023

BYK Apr 11, 2023

BYK Apr 11, 2023

hubertdeng123 Apr 11, 2023

BYK Apr 12, 2023

BYK Apr 11, 2023

hubertdeng123 Apr 11, 2023

stayallive commented Apr 17, 2023

hubertdeng123 commented Apr 17, 2023

stayallive commented Apr 17, 2023

stayallive commented Apr 18, 2023

BYK commented Apr 18, 2023 •

edited

Loading

hubertdeng123 commented Apr 18, 2023

		# Finally, remove the new old volume as we are all in sentry-postgres now
		docker volume rm sentry-postgres-new

Upgrade Postgres to 14 #2074

Upgrade Postgres to 14 #2074

Conversation

hubertdeng123 commented Apr 6, 2023 • edited Loading

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stayallive commented Apr 17, 2023

hubertdeng123 commented Apr 17, 2023

stayallive commented Apr 17, 2023

stayallive commented Apr 18, 2023

BYK commented Apr 18, 2023 • edited Loading

hubertdeng123 commented Apr 18, 2023

hubertdeng123 commented Apr 6, 2023 •

edited

Loading

BYK commented Apr 18, 2023 •

edited

Loading