Skip to content

Conversation

@cbandy
Copy link
Member

@cbandy cbandy commented Sep 26, 2025

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change, as applicable?
  • Have you tested your changes on all related environments with successful results, as applicable?
    • Have you added automated tests?

Type of Changes:

  • New feature

What is the current behavior (link to any open issues here)?

Images for major upgrades must have binaries at Red Hat paths.

What is the new behavior (if this is a feature change)?

Major upgrades look for binaries in typical paths for multiple distro flavors: Alpine, Debian, Red Hat.

Other Information:

Issue: PGO-864

@cbandy cbandy requested a review from jmckulk September 26, 2025 21:30
@cbandy cbandy force-pushed the upgrade-paths branch 3 times, most recently from cdbdad4 to 9ecc14d Compare September 29, 2025 15:11
// - https://cwrap.org/nss_wrapper.html
`export LD_PRELOAD='libnss_wrapper.so' NSS_WRAPPER_GROUP NSS_WRAPPER_PASSWD`,

// Find directories that contain the desired Postgres executables.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe describe what is going on here a little more.

My understanding is that the postgres.ShellPath stuff sets the path variable and the command -v postgres/initdb returns the full path to the binary

Copy link
Member Author

Choose a reason for hiding this comment

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

👍🏻 I've added some description.

That made me realize the PATH built into the image is also searched, so any ol' postgres might be found. I added an assertion* on the --version returned by something in old_bin and new_bin, and learned and doc'd that pg_upgrade does a similar check only recently-ish.

* same check as the postgres init container:

`results 'postgres version' "${postgres_version:=$(postgres --version ||:)}"`,
`[[ "${postgres_version}" =~ ") ${expected_major_version}"($|[^0-9]) ]] ||`,
`halt Expected PostgreSQL version "${expected_major_version}"`,

@sfc-gh-jmckulka
Copy link
Contributor

With this change do we need the postgres bins on the PATH in the image level 🤔

This leaves the disk untouched when the upgrade image cannot support the requested upgrade. Issue: PGO-300 See: 406e069
@cbandy cbandy marked this pull request as draft September 29, 2025 17:29
I find this Go code and resulting Bash easier to read. This also logs more about what is happening without changing the sequence of commands. The script included an unnecessary `|| exit`, so I moved the `set -eu` out of the Bash invocation into the script itself to make that behavior more obvious.
This makes major upgrades compatible with images from other distros. Issue: PGO-864
@cbandy cbandy marked this pull request as ready for review September 29, 2025 19:27
@cbandy
Copy link
Member Author

cbandy commented Sep 29, 2025

With this change do we need the postgres bins on the PATH in the image level 🤔

PGUpgrade (upgrade image) should work without it after this, but PostgresCluster (postgres image) still relies on the image PATH.

@cbandy cbandy requested a review from benjaminjb September 29, 2025 20:00
@cbandy cbandy merged commit b1b6652 into CrunchyData:main Sep 29, 2025
19 checks passed
@cbandy cbandy deleted the upgrade-paths branch September 29, 2025 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants