Hi Daniel, Thanks for the review. v3 attached.
*On external wrapper vs. in-tree:* the point of --initdb is deriving the new cluster's settings (WAL segment size, checksums, encoding, locale) from the old cluster, exactly what people get wrong by hand and only discover when check_control_data() fails. pg_upgrade already reads the old control data, so doing it here reuses that rather than making a wrapper rediscover it. I'm open to being convinced otherwise. The rest: *- --check --initdb:* you're right it was broken (ran before live-check was determined). Since --check is read-only and --initdb creates the cluster, the combination doesn't make sense — v3 rejects it at option parsing, which removes the live-check problem entirely. *- get_control_data() twice:* the early --initdb read populates the old cluster's control data, so the later call in check_cluster_compatibility() returns early when ctrl_ver is already set. New cluster is still read fresh. *- adjust_data_dir(): *moved adjust_data_dir(&new_cluster) before the initdb call so a config-only -D is resolved first. *- check_bindir(): *meant get_bin_version(). Fixed. *- Docs:* noted that only initdb-accepted settings are configured, postmaster-only options need manual creation, and the --check restriction. TAP test covers the --check rejection. pgindent-clean, tests pass. https://github.com/LeeBohyun/postgres/tree/pg_upgrade_initdb Best regards, Bohyun On Tue, Jul 14, 2026 at 12:06 PM Daniel Gustafsson <[email protected]> wrote: > I am not convinced that all this complexity and overhead isn't better > suited > for external wrappers to pg_upgrade like Debian's pg_upgradecluster etc. > > + if (user_opts.initdb_new_cluster) > + create_new_cluster_via_initdb(); > + > adjust_data_dir(&new_cluster); > > If pg_upgrade controls how initdb was invoked for the new cluster, > shouldn't it > work such that adjust_data_dir isn't required? > > > + /* > + * get_control_data() selects pg_resetwal vs. pg_resetxlog via > + * bin_version, which check_bindir() normally fills in later. > Seed it now > + * so the right binary name is used in this early call. > + */ > + if (old_cluster.bin_version == 0) > + old_cluster.bin_version = old_cluster.major_version; > + > + get_control_data(&old_cluster); > > This can't be done unconditionally, the old cluster can still be running at > this point. For example if someone wants to do a live-check: > > $ ./bin/pg_upgrade -b ./bin/ -B ./bin/ -d ./data_old/ -D ./data_new/ > --check --initdb > > The source cluster was not shut down cleanly, state reported as: "in > production" > Failure, exiting > > get_control_data is big, expensive, and really designed to be run once. I > don't think it's Ok to run it an extra time here without at least being > able to > tell the later invocation that it has already been executed. Also, > check_bindir() as referred to in the comment does not exist. > > > + prep_status("Inspecting old cluster locale for new cluster creation"); > + start_postmaster(&old_cluster, true); > + get_template0_info(&old_cluster); > + stop_postmaster(false); > + check_ok(); > > Again, cannot be done unconditionally. > > > + * Users needing options that only the postmaster accepts can > create the > + * new cluster manually and omit --initdb. > > This should probably be expanded upon in the documentation. > > -- > Daniel Gustafsson > >
v3-pg_upgrade-initdb.patch
Description: Binary data
