Thanks, Demir, for v5 and the work on the orphan-directory cleanup and the --check dry run, and thanks Daniel and Greg for the continued
review. Attached is v6. It builds on Demir's v5 and folds in the remaining points below. Per Daniel's suggestion it is now split: 0001 - docs only: the manual "Initialize the new cluster" clarification 0002 - the --initdb feature (Demir's v5 plus the fixes below) Responses are all inline. On Tue, Aug 12, 2026, Daniel Gustafsson wrote: > + appendPQExpBuffer(&cmd, " -U \"%s\"", os_info.user); > > This will inject the username specified in the (untrusted input) > commandline into the initdb command and execute it. [...] it seems > like a Booby Tables pattern [...] > The initdb command is now assembled with appendShellString() for every externally-derived value (i.e., the data directory, username, bindir, and the encoding/locale strings), so shell metacharacters can no longer break out of an argument. > Regardless of the rest of the patch, I think we should apply > something like this to the docs. I propose you extract this portion > into a separate 0001 in this patchset so that it can be considered > individually. > 0001 is docs-only and stands on its own. It also corrects the earlier implication that encoding and locale must match -- pg_upgrade copies those from the old cluster, so only the WAL segment size and the data-checksum setting actually need to match, which 0001 now states with a pointer to pg_controldata. On Wed, Aug 13, 2026, Greg Sabino Mullane wrote: > I see your point. Maybe we don't allow --check with --initdb at all? > v5 resolved this as a dry run rather than a hard block. --check --initdb reports the initdb command it would run and validates the preconditions (correct new-binary version, empty target directory, old cluster reachable) without creating the new cluster. v6 keeps that, and the docs now describe it. > I'm not clear what pointing this out for --initdb only accomplishes. > A wrong -B is going to cause problems regardless of the mode. > I agree, and I believe this meets Demir's point about orphan directories. v5 checks the new-binary version up front, so a wrong -B fails before anything is written. An atexit handler removes the created directory if a later check fails, up until the point where real data has been copied. So there is no need for a -B note specific to --initdb in the docs. v6 carries both. > Eliminating what exactly? And why would a vacuum or checkpoint cause > problems? > The old server is started here with the same start_postmaster() that pg_upgrade uses elsewhere -- including -b, which disables autovacuum -- only to read template0's encoding and locale, then stopped before the new cluster is created. So autovacuum will not run, and a checkpoint would be harmless. It is a brief, read-only start using pg_upgrade's existing mechanism, so it adds no risk beyond the old-server starts pg_upgrade already performs. The docs now mention this brief start. On Sun, Aug 17, 2026, Hüseyin Demir wrote: > a log message in the new --check --initdb dry-run path had a trailing > newline character it shouldn't have had [...] Fixed [...] > Carried into v6. Beyond the review points, v6 also tightens two things I noticed while reworking this: - The empty-directory guard now rejects any non-empty target directory, not just one already containing a PG_VERSION file. This closes a gap in the orphan-cleanup path. The cleanup handler removes the whole new data directory on failure, so it must not run against a directory that already held the user's files. - Because --initdb reads template0 by briefly starting the old server, it now checks in advance that the old server is shut down and fails with the usual "shut down that postmaster" message, rather than failing later when pg_resetwal refuses to run against a live server. Branch on GitHub: github.com/LeeBohyun/postgres/tree/pg_upgrade_initdb Best regards, Bohyun On Mon, Aug 17, 2026 at 9:44 AM Hüseyin Demir <[email protected]> wrote: > > 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. > > > > One more thing. While checking this against the CI build I found one > more small bug: a log message in the new --check --initdb dry-run path > had a trailing newline character it shouldn't have had, which tripped > an internal safety check and caused a crash — but only on that > stricter CI build, not on a normal build. Fixed by removing the extra > newline. v5 is now confirmed working on both build types. >
v6-0002-pg_upgrade-initdb.patch
Description: Binary data
v6-0001-pg_upgrade-initdb-doc.patch
Description: Binary data
