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

Hi Bohyun,

I applied v6 on master (798bdcae89d) and tested it. The normal path
works. I made the old cluster with --no-data-checksums --wal-segsize=32
--locale=C and the new cluster got all three right.

0001 is a good doc fix by itself. It could go in separately.

I found six things in 0002. A script that reproduces all of them is
attached. Run it as "sh initdb_repro.sh /path/to/bin /path/to/srcdir".

1. t/009_initdb_option.pl is not listed in
src/bin/pg_upgrade/meson.build, so it never runs in a meson build. I
added the line and all 26 subtests pass.

2. The group access setting from initdb -g is not copied from the old
cluster. The old cluster is drwxr-x--- and the new one is drwx------.
The exit code is 0 and there is no warning. The checksum and WAL
segment size are checked by pg_upgrade, but nothing checks this one, so
the upgrade quietly drops something the user asked for. Any tool that
reads the data directory as a member of the postgres group will stop
working.

3. resolve_new_bindir() is a nice cleanup, but the new callers pass
os_info.progname. That is get_progname(argv[0]), so it has no directory
in it. find_my_exec() then searches PATH instead of using argv[0]. If
the install directory is not in PATH and -B is not given, --initdb stops
with "could not find own program executable". The same command without
--initdb works fine. This is the fallback that lets people skip -B, so
it should get argv[0], like setup() does.

4. The <pgdata>.initdb_log directory is never deleted. It stays after a
successful run and after --check, and --retain has no effect on it.
Everywhere else pg_upgrade puts its logs inside the new data directory
and deletes them in cleanup_output_dirs(). Putting this one next to
pgdata also means the parent directory has to be writable, which
pg_upgrade does not need today.

5. A small note on the empty directory check. It stops initdb from
running where there are files, but the cleanup still calls rmtree() on
the directory itself. So a directory the operator made by hand is gone
after a failed run. Nothing is lost, since it had to be empty. But if
only the contents were deleted, initdb could still retry and the
directory would keep its owner and mode.

6. Two things about --check --initdb. I saw that v5 turned this into a
dry run instead of blocking it. First, the live check problem Daniel
reported for v2 is back. With the old cluster running, plain --check
says "Clusters are compatible", while --check --initdb says "There seems
to be a postmaster servicing the old cluster". Second, the dry run
returns 0 for a cluster that cannot be upgraded. With a regproc column
in the old cluster, --check --initdb returns 0 and the real run returns
1.

I am happy to write patches for any of these.

Thanks,
Shihao