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

From: Greg Sabino Mullane <htamfids(at)gmail(dot)com>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: Bohyun Lee <bohyun(dot)lee(at)databricks(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically
Date: 2026-07-16 18:17:01
Message-ID: CAKAnmmKb7o9iWCXEt0uY=pZwo50OZD=E=Hm+f18DZM+h7+U-dw@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2026-07-16 19:22:44 Re: Follow-up fixes for online wal_level change
Previous Message Sami Imseih 2026-07-16 18:14:59 Re: Improve pg_stat_statements scalability