Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically

Hi,

A few feedback about introducing the initdb option to pg_upgrade.

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
```

So if the check is enabled it should print out the potential initdb
command/arguments/output and run the check. Because it can lead to
misunderstanding while executing the actual upgrade commands. And
operator/script has to remove the initdb directory before actual
upgrade.

When it comes to documentation we also need to add that the initdb
option starts the old server briefly to discover settings from
template0, then stops it before creating the new cluster. Moreover,
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.

And the --initdb behaviour will introduce additional risks during the
upgrade. During the initdb the settings discovery connections are
allowed on database level and we don't have a guarantee that
checkpoint and vacuums won't start. This needs to be documented
clearly or we should consider eliminating them.

Regards,
Demir.

Bohyun Lee <bohyun(dot)lee(at)databricks(dot)com>, 17 Tem 2026 Cum, 14:10
tarihinde şunu yazdı:
>
> Thanks both for the reviews. v4 attached, addressing all of Greg's second-pass points and Daniel's documentation point.
>
> Greg:
>
> resolve_new_bindir(): dropped the last two comment lines.
>
> prep_status text is now "Examining old cluster settings".
>
> Test: since checksums are now the default, the old cluster is created with --no-data-checksums (a genuinely non-default value) and --locale=C. The post-upgrade checks confirm the new cluster inherited both, along with the WAL segment size, encoding, collation, ctype, and locale provider.
>
> Test: replaced open()/print() with append_conf, and the port/socket settings now follow the framework's TCP-vs-Unix logic ($use_tcp), so Windows works too.
>
> -O: dropped support with --initdb entirely, as you originally suggested. The partial "-c only" forwarding still broke on quoted values with spaces. pg_upgrade now rejects -O + --initdb during option parsing, with a test and a doc note.
>
>
> Daniel:
>
> Reworked the manual-initdb docs: the vague "use compatible initdb flags" is now the specific settings that must match (WAL segment size, checksums, encoding, locale), with a pointer to pg_controldata.
>
> On in-tree vs. wrapper: these settings have to come from the old cluster's control data, which pg_upgrade already reads, a wrapper would have to rediscover the same information, and getting it wrong is exactly the failure --initdb is meant to prevent.
>
>
> The patch is pgindent-clean and the full pg_upgrade TAP suite passes (including the new t/009_initdb_option.pl).
>
> Registered in PG20-2. Branch on GitHub: https://github.com/LeeBohyun/postgres/tree/pg_upgrade_initdb
>
>
> Best regards,
>
> Bohyun
>
>
> On Thu, Jul 16, 2026 at 8:17 PM Greg Sabino Mullane <htamfids(at)gmail(dot)com> wrote:
>>
>> Second review pass.
>>
>> Cleanly compiles and all tests pass when applied to 637aa273e (Thu Jul 16 11:56:01 2026 -0400)
>>
>> /*
>> * resolve_new_bindir()
>> *
>> * Idempotent helper: if new_cluster.bindir has not been set by the user via
>> * -B, derive it from the path of the currently executing pg_upgrade binary.
>> * Called early by create_new_cluster_via_initdb() so that the initdb path
>> * is available before verify_directories() runs.
>> */
>>
>> Do we really need the last two lines? This is also called during the "normal" path, not just via create_new_cluster_via_initdb, so not sure why we are pointing it out here.
>>
>>
>> > prep_status("Inspecting old cluster locale for new cluster creation");
>>
>> We are doing a lot more than locale though. How about:
>>
>> prep_status("Examining old cluster settings");
>>
>>
>> > # Use non-default settings that --initdb must carry over to the new cluster
>> > # (derived from the old cluster's pg_control): data checksums and a non-default
>> > # WAL segment size. We check below that the new cluster inherits them.
>> > my $oldnode = PostgreSQL::Test::Cluster->new('old_node');
>> > $oldnode->init(extra => [ '--data-checksums', '--wal-segsize' => '2' ]);
>>
>> But checkums ARE the default now, so for a non-default setting, we should do --no-data-checksums. Would be nice to check locale as well. That gets tricky, but maybe --locale=C is nicely portable and pretty non-default?
>>
>>
>> > my $conf = $newnode->data_dir . '/postgresql.conf';
>> > open(my $fh, '>>', $conf) or die "could not open $conf: $!";
>>
>> Easier to use $newnode->append_conf
>>
>>
>> > print $fh "listen_addresses = ''\n";
>> > print $fh "unix_socket_directories = '" . $newnode->host . "'\n";
>>
>> What about Win32? See .e.g $use_tcp in Cluster.pm
>>
>> > * Forward only "-c name=value" options from -O to initdb. initdb accepts
>>
>> I still think we just bail on supporting -O. The new code is an improvement, but fails if there are legitimate spaces in any of the args:
>>
>> $ pg_upgrade --initdb -O "-c transaction_isolation='repeatable read'"
>>
>> Creating new cluster with initdb ignoring non-"-c" option(s) passed via -O; only "-c" settings are forwarded to initdb for --initdb
>> sh: 1: Syntax error: Unterminated quoted string
>>
>> *failure*
>>
>>
>> Cheers,
>> Greg