-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Change PGDATA in 18+ to /var/lib/postgresql/MAJOR/docker
#1259
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
Conversation
|
|
|
I'm not sure how we help catch cases of users blindly upgrading to 17+ with a volume of |
$ docker run -it --rm -v foo:/var/lib/postgresql/data --name foo --env POSTGRES_PASSWORD=foo 8b5ecf7fdad7
docker: Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error mounting "/var/lib/docker/volumes/foo/_data" to rootfs at "/var/lib/postgresql/data": change mount propagation through procfd: open o_path procfd: open /var/lib/docker/overlay2/bd954e9c05618d52115b5345f7465cf17cc426560b0979d7f796ebfbf62ea950/merged/var/lib/postgresql/data: no such file or directory: unknown.Ouch, nope, symlink is a bust. |
|
I mean, on the plus side, it did make the container runtime fail to start the container, so maybe that's better than silently and quietly failing? 🤔 |
|
Oh, I missed the most obvious way to mitigate this change for existing users: just set (OP updated) |
ef9b704 to
c059fba
Compare
It's an interesting idea. Trying to figure out how that's beneficial for containers, but nothing super useful is popping up for me. What's the concept you have in mind? 😄 |
|
It's not directly useful so much as "more in line with the common standards used in this pre-existing ecosystem" -- us using a non-standard path has been a source of friction for our users since we first introduced the image. This gets especially focused when you try to use |
|
Interesting. My first thought is that embedding the version number in the path like this would probably make adding some kind of automatic upgrade images much more difficult. At least, the approach we're using with the pgautoupgrade repo is to upgrade things in place. Might be able to work around such a change with a bit of extra shell scripting though. I guess the 2nd worry I have is that this change is breaking the many years of following a particular approach (and thereby needing people to update their custom tooling to match), mostly for what seems like an arbitrary change without a clear benefit. Those kind of changes tend to cause follow on headaches for people. 😦 |
|
The workaround is to set |
|
Cool, yeah that should work for people with existing images and tooling. With this:
I'm trying to understand the pain/failure scenario there. Are there good examples of this problem in this repo that I can read through to better understand? With this new path structure, it seems to be matching the path layout which Debian has implemented for its PostgreSQL packaging. So I can sort of understand where you're coming from with respect to implementing it for the Debian based images. Alpine doesn't seem to use that path structure for its PostgreSQL packages though? |
It does seem to use the same paths as noted by a user and when I install and run the / # apk add postgresql14
...
/ # rc-service postgresql start
/ # rc-service postgresql stop
/ # ls -l /var/lib/postgresql/*/data/PG_VERSION
-rw------- 1 postgres postgres 3 Aug 19 21:49 /var/lib/postgresql/14/data/PG_VERSION
/ # apk add postgresql15
...
/ # pg_versions set-default 15
/ # rc-service postgresql start
/ # ls -l /var/lib/postgresql/*/data/PG_VERSION
-rw------- 1 postgres postgres 3 Aug 19 21:49 /var/lib/postgresql/14/data/PG_VERSION
-rw------- 1 postgres postgres 3 Aug 19 21:50 /var/lib/postgresql/15/data/PG_VERSION
I think it is partly that the Current optimal usage: $ find DIR -mindepth 2 -maxdepth 2
DIR/OLD/data
DIR/NEW/data
$ # "OLD" server was run with a specific subdir next to where next version will store data
$ docker run -it --rm \
-v DIR/OLD/data:/var/lib/postgresql/data \
postgres:OLD
$ docker run --rm \
-v DIR:/var/lib/postgresql \
tianon/postgres-upgrade:OLD-to-NEW \
--link
$ # NEW server with the new directory
$ docker run -d \
-v DIR/NEW/data:/var/lib/postgresql/data \
postgres:NEWSlow upgrade with volumes (or unable to use the OLD directory parent or unable to move the OLD data directory): $ docker run -it --rm \
-v PGDATAOLD:/var/lib/postgresql/data \
postgres:OLD
$ docker run --rm \
-v PGDATAOLD:/var/lib/postgresql/OLD/data \
-v PGDATANEW:/var/lib/postgresql/NEW/data \
tianon/postgres-upgrade:OLD-to-NEW
$ docker run -d \
-v PGDATANEW:/var/lib/postgresql/data \
postgres:NEWUsing the newer layout from this proposal will mean that running or upgrading can use the same mount path (or a volume): $ # "old" server
$ docker run -it --rm \
-v DIR:/var/lib/postgresql \
postgres:17
$ # upgrade
$ docker run --rm \
-v DIR:/var/lib/postgresql \
tianon/postgres-upgrade:17-to-18 \
--link
$ # new server
$ docker run -d \
-v DIR:/var/lib/postgresql \
postgres:18 |
|
Drawback of this proposal is that if a user runs a new major PostgreSQL version without upgrading (like |
Hmmm, that'd probably be fixable if the start up script checks for that situation and moves things around automatically. |
Couldn't the startup script just error out if it finds an existing older version database, or automatically upgrade that database to the current version if some enviromental variable, for example |
The automatic upgrade thing would definitely benefit a lot of people. Not sure if the maintainers here are interested in including that (note the complete lack of response), though I could be wrong. |
Thanks for the link. Acording to tianon there:
And also:
This seems to be the main roadblock. |
To me, that seems a bit silly. I mean, that's why I was suggesting having PG images that don't do "automatic upgrade" as they'd just be the "normal" size. But for people that do need an automatically upgrading image - unlikely to be everyone? - then sure it's going to be a bit bigger. They don't seem all that huge though. For reference, these are the standard PostgreSQL images: https://hub.docker.com/_/postgres/tags They're about 140-150MB (roughly). These are the "automatic upgrade" images at the pgautoupgrade project: https://hub.docker.com/repository/docker/pgautoupgrade/pgautoupgrade/tags They're about 190MB (roughly). So, ~40MB larger (~25%) due to including the binaries for several PG versions in them. Seems like a pretty decent trade off (to me). 😄 |
Yep, I agree that an automatic upgrade Postgres docker container would be amazing. I can see why you might not want it to be the default, but it would be very helpful to have as an option. We use the Postgres docker container in our local development environments, and every Postgres version upgrade is a pain that every developer needs to deal with (until I discovered pgautoupgrade today). |
|
Automatic upgrade is orthogonal to this PR (and in fact, this PR should make it more straightforward to do safely, although I'll reiterate that we have no plans to implement/accept/maintain such functionality directly).
Yeah, that's probably the approach I'd use (to error out, not move anything, since the whole point is that the data files for 17 aren't compatible with 18 and the data files for pre-releases of 18 aren't even officially compatible with GA). With 18 now in pre-release (#1344), this is probably worth updating / considering again. |
Let's do it. We haven't released an 18 yet and can rebase this once that is merged. I guess the only addition needed here is "the startup script should error out if it finds an existing older version database". |
PGDATA in 17+ to /var/lib/postgresql/MAJOR/dockerPGDATA in 18+ to /var/lib/postgresql/MAJOR/docker
|
Updated to be 18+ -- now looking at the entrypoint change (I'm happy to rebase this on top of #1344 if that makes it easier to review/consume). |
| elif [ "$PGDATA" = "/var/lib/postgresql/$PG_MAJOR/docker" ]; then | ||
| # https://github.com/docker-library/postgres/pull/1259 | ||
| for d in /var/lib/postgresql /var/lib/postgresql/data /var/lib/postgresql/*/docker; do | ||
| if [ -s "$d/PG_VERSION" ]; then | ||
| OLD_DATABASES+=( "$d" ) | ||
| break | ||
| fi | ||
| done | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's honestly kind of tempting to generalize this and just look for PG_VERSION anywhere in /var/lib/postgresql if PGDATA is under that directory, but I opted to at least start with the more conservative version that will only error out on image upgrades that don't also upgrade the database explicitly in the default configuration (or if they blindly update their VOLUME to match ours so now we have a database at /var/lib/postgresql directly).
docker-entrypoint.sh
Outdated
| for d in /var/lib/postgresql /var/lib/postgresql/data /var/lib/postgresql/*/docker; do | ||
| if [ -s "$d/PG_VERSION" ]; then | ||
| OLD_DATABASES+=( "$d" ) | ||
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This break seems out of place if you want a list of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, indeed -- I started with a single string, then realized that if I'm looping, I might as well let this very low-overhead loop complete and gather all the directories so we can tell the user the full story.
|
As clarified in #1372, the concrete fix to your definition there is to adjust your volume from |
it appears that we need to update the DB volume mount directory, however that is something which started to be enforced recently, see: docker-library/postgres#1376 Error: in 18+, these Docker images are configured to store database data in a format which is compatible with "pg_ctlcluster" (specifically, using major-version-specific directory names). This better reflects how PostgreSQL itself works, and how upgrades are to be performed. See also docker-library/postgres#1259 Counter to that, there appears to be PostgreSQL data in: /var/lib/postgresql/data (unused mount/volume) This is usually the result of upgrading the Docker image without upgrading the underlying database using "pg_upgrade" (which requires both versions). The suggested container configuration for 18+ is to place a single mount at /var/lib/postgresql which will then place PostgreSQL data in a subdirectory, allowing usage of "pg_upgrade --link" without mount point boundary issues. See docker-library/postgres#37 for a (long) discussion around this process, and suggestions for how to do so.
it appears that we need to update the DB volume mount directory, however that is something which started to be enforced recently, see: docker-library/postgres#1376 Error: in 18+, these Docker images are configured to store database data in a format which is compatible with "pg_ctlcluster" (specifically, using major-version-specific directory names). This better reflects how PostgreSQL itself works, and how upgrades are to be performed. See also docker-library/postgres#1259 Counter to that, there appears to be PostgreSQL data in: /var/lib/postgresql/data (unused mount/volume) This is usually the result of upgrading the Docker image without upgrading the underlying database using "pg_upgrade" (which requires both versions). The suggested container configuration for 18+ is to place a single mount at /var/lib/postgresql which will then place PostgreSQL data in a subdirectory, allowing usage of "pg_upgrade --link" without mount point boundary issues. See docker-library/postgres#37 for a (long) discussion around this process, and suggestions for how to do so.
Pgsql 18 will lead some error. This is a temporary fix. Reference: docker-library/postgres#1259
|
THIS: postgres-data:/var/lib/postgresql WORKS FOR ME |
…rmat This fixes the startup error saying: ``` In 18+, these Docker images are configured to store database data in a format which is compatible with "pg_ctlcluster" (specifically, using major-version-specific directory names). This better reflects how PostgreSQL itself works, and how upgrades are to be performed. See also docker-library/postgres#1259 Counter to that, there appears to be PostgreSQL data in: /var/lib/postgresql/data (unused mount/volume) This is usually the result of upgrading the Docker image without upgrading the underlying database using "pg_upgrade" (which requires both versions). The suggested container configuration for 18+ is to place a single mount at /var/lib/postgresql which will then place PostgreSQL data in a subdirectory, allowing usage of "pg_upgrade --link" without mount point boundary issues. ``` Signed-off-by: Sebastian Schuberth <[email protected]>
…lume mount See the comment at [1]. This fixes the startup error saying: ``` In 18+, these Docker images are configured to store database data in a format which is compatible with "pg_ctlcluster" (specifically, using major-version-specific directory names). This better reflects how PostgreSQL itself works, and how upgrades are to be performed. See also docker-library/postgres#1259 Counter to that, there appears to be PostgreSQL data in: /var/lib/postgresql/data (unused mount/volume) This is usually the result of upgrading the Docker image without upgrading the underlying database using "pg_upgrade" (which requires both versions). The suggested container configuration for 18+ is to place a single mount at /var/lib/postgresql which will then place PostgreSQL data in a subdirectory, allowing usage of "pg_upgrade --link" without mount point boundary issues. ``` [1]: docker-library/postgres#1259 (comment) Signed-off-by: Sebastian Schuberth <[email protected]>
See the comment at [1]. This fixes the startup error saying: ``` In 18+, these Docker images are configured to store database data in a format which is compatible with "pg_ctlcluster" (specifically, using major-version-specific directory names). This better reflects how PostgreSQL itself works, and how upgrades are to be performed. See also docker-library/postgres#1259 Counter to that, there appears to be PostgreSQL data in: /var/lib/postgresql/data (unused mount/volume) This is usually the result of upgrading the Docker image without upgrading the underlying database using "pg_upgrade" (which requires both versions). The suggested container configuration for 18+ is to place a single mount at /var/lib/postgresql which will then place PostgreSQL data in a subdirectory, allowing usage of "pg_upgrade --link" without mount point boundary issues. ``` [1]: docker-library/postgres#1259 (comment) Signed-off-by: Sebastian Schuberth <[email protected]>
See the comment at [1]. This fixes the startup error saying: ``` In 18+, these Docker images are configured to store database data in a format which is compatible with "pg_ctlcluster" (specifically, using major-version-specific directory names). This better reflects how PostgreSQL itself works, and how upgrades are to be performed. See also docker-library/postgres#1259 Counter to that, there appears to be PostgreSQL data in: /var/lib/postgresql/data (unused mount/volume) This is usually the result of upgrading the Docker image without upgrading the underlying database using "pg_upgrade" (which requires both versions). The suggested container configuration for 18+ is to place a single mount at /var/lib/postgresql which will then place PostgreSQL data in a subdirectory, allowing usage of "pg_upgrade --link" without mount point boundary issues. ``` [1]: docker-library/postgres#1259 (comment) Signed-off-by: Sebastian Schuberth <[email protected]>
|
@tianon Could you consider updating the |
|
Good point, thank you! Updated in tianon/docker-postgres-upgrade#120 |
…rding to 18.0+ versions docker-library/postgres#1259
This is a pretty large breaking change, which is why this only makes the change in 18+ (which is currently in pre-release stages, and not due for GA until September, and pre-release
PGDATAdirectories are officially not supported on the GA release anyhow).Concretely, this changes
PGDATAto/var/lib/postgresql/MAJOR/docker, which matches the pre-existing convention/standard of thepg_ctlcluster/postgresql-commonset of commands, and frankly is what we should've done to begin with, in a classic case of Chesterton's Fence.This also changes the
VOLUMEto/var/lib/postgresql, which should be more reasonable, and make the upgrade constraints more obvious.For any users who have been testing the pre-releases, the simplest way to keep your existing data directory is going to be to addPGDATA=/var/lib/postgresql/dataas an environment variable on your container or adjust your bind-mount from/var/lib/postgresql/datato/var/lib/postgresql/18/docker, but the best way is going to be to refactor your host directory such that your data lives at18/dockerinside and you can then mount directly to/var/lib/postgresql(possibly settingPGDATA=/var/lib/postgresql/MAJOR/dockeras well, if you want to go overboard on being explicit).Users who wish to opt-in to this change on older releases can do so by setting
PGDATAexplicitly (--env PGDATA=/var/lib/postgresql/17/docker --volume some-postgres:/var/lib/postgresql). To migrate pre-existing data, adjust the volume's folder structure appropriately first (moving all database files into aPG_MAJOR/dockersubdirectory).