Hi, >> When I run the pg_upgrade command with the following options I'd >> expect it to not create a directory and initdb. >> >> ``` >> pg_upgrade --check --initdb -d /old -D /new >> ``` > > > I see your point. Maybe we don't allow --check with --initdb at all?
Yes, that would make sense. My point is that, we should check if --initdb is possible or not when the operator/tool executes it via the --check option. >> also mention that -B flag points outs to target version correctly >> since there is a risk of doing an initdb with the wrong version. If -B >> points to an older version, pg_upgrade may create the wrong cluster. > > > I'm not clear what pointing this out for --initdb only accomplishes. A wrong > -B is going to cause problems regardless of the mode. My point is that --new-bindir at the wrong major version's initdb, run pg_upgrade --initdb. The wrong-version initdb succeeds and populates new_cluster.pgdata fully. Then check_cluster_versions() fires pg_fatal(), and the process exits with a fully-initialized, wrong-version, orphaned data directory on disk — and no cleanup code runs anywhere in the patch. We need to enhance the patch so once it fails it should also clean up the orphan directories. Because current failure in pg_upgrade doesn't lead to orphan directories. Otherwise, if pg_upgrade fails and operator wants to execute it again it won't be able to do it since the directory already is not empty and initdb won't work. We need to decide if pg_upgrade will wipe out orphan initdb directories if pg_upgrade fails or leave it to the operator. Summary of v5 changes v4 had a shortcut that skipped re-reading the old cluster's control data if it had already been read once. Turns out this was skipping a real read later in the process that the upgrade actually depends on. Removed the shortcut since it wasn't needed anyway, reading control data is cheap. Fixed the orphan directory problem. This was the main issue from the review. If --initdb creates the new cluster but something fails afterward (wrong binary version, etc.), the directory used to get left behind, and a retry would fail because pg_upgrade refuses to overwrite it. Now we automatically clean it up if something goes wrong, but only up until the point where real data has actually been copied over. After that, we leave it alone, because deleting real data is worse than leaving an orphan folder. --check --initdb now works as a dry run, instead of being blocked. It tells you whether --initdb would succeed, checking the right binary version, an empty target folder, and that the old cluster is reachable, without actually creating anything. Added a version check up front. As part of #3, we now check the new binary's version before doing anything else. This is actually what stops the orphan directory problem from happening in the first place, not just for the dry run. If the version is wrong, we fail immediately, before touching the disk. Updated the docs to explain all of the above. Ran the full build and test suite. New test file covers all the scenarios above, 26 out of 26 passing, no regressions elsewhere. Patch attached. I've renamed the patch file to v5-0001-pg_upgrade-initdb.patch to match our naming convention. No content change, just the filename. Happy to walk through any of this in more detail. Regards, Demir.
v5-0001-pg_upgrade-initdb.patch
Description: Binary data
