Upgrade flow for images RHEL/CentOS 8+ #562

eifrach · 2024-04-11T12:11:53Z

Creating flow for upgrading postgresSQL

add install_weak_deps=false to installation which reduce the image size
install postgresql-upgrade package for the upgrade

eifrach · 2024-04-11T12:12:17Z

[test]

fila43 · 2024-04-11T15:39:15Z

13/root/usr/share/container-scripts/postgresql/common.sh

@@ -297,7 +297,7 @@ function wait_for_postgresql_master() {
 run_pgupgrade ()
 (
  # Remove .pid file if the file persists after ugly shut down
-  if [ -f "$PGDATA/postmaster.pid" ] && ! pgrep -f "postgres" > /dev/null; then


Why are you removing the condition? We do not want to remove .pid file of the running process

So I removed it because I had some issues when I was testing the upgrade. I will look into it deeper.
we should check if the PID is up first

so this is from a running container I added a stop before the line
seem that the service is not running, also there isn't any open socket

I'm assuming the it is partially match run-postgresql

bash-4.4$ pgrep -f "postgres" 1 17 ps -ef UID PID PPID C STIME TTY TIME CMD postgres 1 0 0 12:18 pts/0 00:00:00 /bin/bash /usr/bin/run-postgresql postgres 17 1 0 12:18 pts/0 00:00:00 /bin/bash /usr/bin/run-postgresql postgres 18 17 0 12:18 pts/0 00:00:00 /usr/bin/coreutils --coreutils-prog-shebang postgres 19 0 0 12:19 pts/1 00:00:00 bash postgres 27 19 0 12:22 pts/1 00:00:00 ps -ef

how about

Suggested change

if [ -f "$PGDATA/postmaster.pid" ] && ! pgrep -f "postgres" > /dev/null; then

if [ -f "$PGDATA/postmaster.pid" ] && ! pg_isready > /dev/null; then

makes sense +1

fila43 · 2024-04-11T15:46:31Z

13/root/usr/share/container-scripts/postgresql/common.sh

@@ -352,7 +352,6 @@ run_pgupgrade ()
  info_msg "Starting old postgresql once again for a clean shutdown..."
  "${old_pgengine}/pg_ctl" start -w --timeout 86400 -o "-h ''"
  info_msg "Waiting for postgresql to be ready for shutdown again..."
-  "${old_pgengine}/pg_isready"


Why are you removing this safety procedure?

On my testing it returned exit code 2 - I did manual checks the socket was up and running but the pg_isready wasn't catching it.
On the above line we use -w which is waiting for the DB to be ready. I thought maybe we don't need to double check it unless there is another reason for it?

after some searching, It seems we start the server as local only -- which mean it doesn't create a TCP port instead it creates a Linux socket.

the 'pg_isready' tries the port and fails

fila43 · 2024-04-11T15:57:03Z

13/root/usr/share/container-scripts/postgresql/common.sh

  if test "$old_raw_version" = 92; then
    old_collection=postgresql92
  else
-    old_collection=rh-postgresql$old_raw_version
+    old_collection=postgresql-$old_raw_version
  fi


This does not correspond with the current state of PostgreSQL rpm names.

fila43 · 2024-04-11T16:00:45Z

Overall, it looks good. Thank you for your contribution. We will need to generalize it and implement it in the master branch.
https://github.com/sclorg/postgresql-container/blob/master/src/root/usr/share/container-scripts/postgresql/common.sh

pkubatrh · 2024-04-12T12:32:48Z

13/Dockerfile.c8s

@@ -44,7 +44,8 @@ COPY root/usr/libexec/fix-permissions /usr/libexec/fix-permissions
 RUN yum -y module enable postgresql:13 && \
    INSTALL_PKGS="rsync tar gettext bind-utils nss_wrapper postgresql-server postgresql-contrib" && \
    INSTALL_PKGS="$INSTALL_PKGS pgaudit" && \
-    yum -y --setopt=tsflags=nodocs install $INSTALL_PKGS && \
+    INSTALL_PKGS="$INSTALL_PKGS procps-ng util-linux postgresql-upgrade" && \
+    yum -y --setopt=tsflags=nodocs --setopt=install_weak_deps=false install $INSTALL_PKGS && \


What difference in packages is there when using this option? I would rather not introduce it for already built image versions if the change is too disruptive.

it not a must, it removes about 100MB of size - those are packages that are extra doesn't change / affect the core application

For the existing images, I would choose the more defensive approach. I would just add the necessary packages.

eifrach · 2024-04-14T13:23:03Z

Overall, it looks good. Thank you for your contribution. We will need to generalize it and implement it in the master branch. https://github.com/sclorg/postgresql-container/blob/master/src/root/usr/share/container-scripts/postgresql/common.sh

I've issue fixes for all the remarks
once they approved I'll create a PR with changes to all common.sh files

fila43 · 2024-04-16T19:53:23Z

13/root/usr/share/container-scripts/postgresql/common.sh

@@ -311,8 +311,8 @@ run_pgupgrade ()
    old_collection=rh-postgresql$old_raw_version
  fi

-  old_pgengine=/opt/rh/$old_collection/root/usr/bin


This could be changed only for RHEL8+ images. The general change would cause problems in rhel7/centos7 containers. Condition needed in distgen source files.

fila43 · 2024-04-16T19:58:39Z

src/root/usr/share/container-scripts/postgresql/common.sh

@@ -312,7 +312,7 @@ run_pgupgrade ()
    old_collection=rh-postgresql$old_raw_version
  fi

-  old_pgengine=/opt/rh/$old_collection/root/usr/bin
+  old_pgengine=/usr/lib64/pgsql/postgresql-$old_raw_version/bin
  new_pgengine=/opt/rh/rh-postgresql${new_raw_version}/root/usr/bin


/usr/bin ?

fila43 · 2024-04-16T20:07:20Z

It makes a good impression on me. Please implement the changes into /src directory. Once it is there, we will check and adjust the compatibility for rhel7/centos7 vs rhel8+ versions, or we can wait for the EOL of centos7/rhel7 images. Also, there should be the prepared upgrade test that needs to be enabled for rhel8+ containers.

eifrach · 2024-04-18T12:44:33Z

[test]

eifrach · 2024-04-18T12:48:21Z

cc @danmanor

fila43 · 2024-04-24T19:54:51Z

LGTM, but there is dist-gen issue

fila43 · 2024-04-24T19:55:17Z

@phracek would it be possible to enable this test also for RHEL8+ ?

postgresql-container/test/run_test

Line 682 in d0cecca

run_upgrade_test ()

fila43 · 2024-04-24T19:55:29Z

[test]

erthalion · 2024-04-26T12:05:03Z

Hi,

Thanks for the PR! I've got a couple of general commentaries about PostgreSQL
major version upgrade, and although they may not necessarily be related to the
PR, I reckon it's a good idea to collect them here. As an outside person I
could be missing some details of this project, please let me know if that's the
case.

POSTGRESQL_UPGRADE seems to allow only --link and --copy, but since PG
11 there is also --clone option, which somewhat combines best of both
worlds, but requires newer Linux kernel versions.
The docs mention --link option as an unsafe, but the old cluster should not
be used only of the new cluster was started. If it wasn't the case, the old
cluster was unmodified and should be safe to start.
It seems that it's possible to provide custom initdb options via
POSTGRESQL_UPGRADE_INITDB_OPTIONS, but some of them must be the same as the
old cluster (collate, encoding, data checksums, wal segment size). It sounds
a bit unreliable to leave it to the container user, could those options be
enforced?
It's worth running pg_upgrade --check to verify compatibility.
Maybe a good idea to add -j to upgrade_cmd for additional parallelism.
Usually triggering a vacuum after the upgrade to rebuild statistics is a
good idea as well.
More open-ended question, what do you think about those situations where
some coordination is needed? E.g. when upgrading a replica, most likely rsync
has to be used in coordination with the primary upgrade. Or upgrading
extensions, which is not handled via pg_upgrade either.

It's probably beyond what you want to achieve within a container, but it's
at least worth mentioning in the documentation, right?

https://www.postgresql.org/docs/current/pgupgrade.html

fila43 · 2024-04-30T13:48:33Z

[test]

fila43 · 2024-04-30T14:27:37Z

[test]

fila43 · 2024-05-06T09:59:36Z

[test]

fila43 · 2024-05-06T13:38:50Z

[test]

fila43 · 2024-05-06T17:01:54Z

@phracek @zmiklank any idea about c9s failures? RHEL9 passed but c9s failed

zmiklank · 2024-05-07T07:54:52Z

@phracek @zmiklank any idea about c9s failures? RHEL9 passed but c9s failed

This is reproducible also in the master branch - so not seem to be related to this PR. Seems like a segfault (double free) from pg_isready binary.

Aren't there any differences in c9s and rhel9 binaries?

zmiklank · 2024-05-07T09:00:09Z

So this may be an postgresql[1] issue which is present only when using newer openssl (3.2.1) that got into c9s, but not into rhel9 yet. postgresql upstream patch seeems to exist already, more info is in the linked mailing thread[1].

[1] https://www.postgresql.org/message-id/CAN55FZ1eDDYsYaL7mv%2BoSLUij2h_u6hvD4Qmv-7PK7jkji0uyQ%40mail.gmail.com

$eifrach$

$@eifrach$ eifrach mentioned this pull request Apr 11, 2024

Upgrade fails due to postmaster.pid #550

Open

fila43 reviewed Apr 11, 2024

View reviewed changes

pkubatrh reviewed Apr 12, 2024

View reviewed changes

$@eifrach$ eifrach requested review from fila43 and pkubatrh April 15, 2024 12:00

fila43 reviewed Apr 16, 2024

View reviewed changes

$@eifrach$

adding upgrade flow for postgress 13

f451ff9

$@eifrach$ eifrach force-pushed the upgrade_psql branch from 1231f2e to 3404821 Compare April 18, 2024 12:42

$@eifrach$ eifrach changed the title ~~[WIP] adding upgrade flow for postgress 13~~ Upgrade flow for images RHEL/CentOS 8+ Apr 18, 2024

$@eifrach$ eifrach marked this pull request as ready for review April 18, 2024 12:44

$@eifrach$

align all versions

9f472f0

$@eifrach$ eifrach force-pushed the upgrade_psql branch from 3404821 to 9f472f0 Compare April 18, 2024 12:59

$@eifrach$ eifrach requested a review from fila43 April 18, 2024 13:15

Fix distgen issue and regenerate versions

c08233f

Fix typo in Dockerfile.fedora

a4d0904

Add postgresql16-upgrade package into fedora dockerfile

64f9d61

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade flow for images RHEL/CentOS 8+ #562

Upgrade flow for images RHEL/CentOS 8+ #562

$@eifrach$ eifrach commented Apr 11, 2024

eifrach commented Apr 11, 2024

fila43 Apr 11, 2024

$@eifrach$ eifrach Apr 14, 2024

$@eifrach$ eifrach Apr 14, 2024

$@eifrach$ eifrach Apr 14, 2024

fila43 Apr 16, 2024

fila43 Apr 11, 2024

$@eifrach$ eifrach Apr 14, 2024

$@eifrach$ eifrach Apr 14, 2024

$@eifrach$ eifrach Apr 14, 2024

fila43 Apr 16, 2024

fila43 Apr 11, 2024 •

edited

$@eifrach$ eifrach Apr 14, 2024

fila43 commented Apr 11, 2024 •

edited

pkubatrh Apr 12, 2024

$@eifrach$ eifrach Apr 14, 2024

fila43 Apr 16, 2024

eifrach commented Apr 14, 2024

fila43 Apr 16, 2024

fila43 Apr 16, 2024

fila43 commented Apr 16, 2024

eifrach commented Apr 18, 2024

eifrach commented Apr 18, 2024

fila43 commented Apr 24, 2024 •

edited

fila43 commented Apr 24, 2024

fila43 commented Apr 24, 2024

erthalion commented Apr 26, 2024

fila43 commented Apr 30, 2024

fila43 commented Apr 30, 2024

fila43 commented May 6, 2024

fila43 commented May 6, 2024

fila43 commented May 6, 2024

zmiklank commented May 7, 2024

zmiklank commented May 7, 2024

	if [ -f "$PGDATA/postmaster.pid" ] && ! pgrep -f "postgres" > /dev/null; then
	if [ -f "$PGDATA/postmaster.pid" ] && ! pg_isready > /dev/null; then

Upgrade flow for images RHEL/CentOS 8+ #562

Are you sure you want to change the base?

Upgrade flow for images RHEL/CentOS 8+ #562

Conversation

eifrach commented Apr 11, 2024

eifrach commented Apr 11, 2024

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

Choose a reason for hiding this comment

fila43 Apr 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fila43 commented Apr 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eifrach commented Apr 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fila43 commented Apr 16, 2024

eifrach commented Apr 18, 2024

eifrach commented Apr 18, 2024

fila43 commented Apr 24, 2024 • edited

fila43 commented Apr 24, 2024

fila43 commented Apr 24, 2024

erthalion commented Apr 26, 2024

fila43 commented Apr 30, 2024

fila43 commented Apr 30, 2024

fila43 commented May 6, 2024

fila43 commented May 6, 2024

fila43 commented May 6, 2024

zmiklank commented May 7, 2024

zmiklank commented May 7, 2024

$@eifrach$ eifrach commented Apr 11, 2024

fila43 Apr 11, 2024 •

edited

fila43 commented Apr 11, 2024 •

edited

fila43 commented Apr 24, 2024 •

edited