| From: | Bohyun Lee <bohyun(dot)lee(at)databricks(dot)com> |
|---|---|
| To: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
| Cc: | harrisontbooth(at)gmail(dot)com, Greg Sabino Mullane <htamfids(at)gmail(dot)com>, Daniel Gustafsson <daniel(at)yesql(dot)se>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically |
| Date: | 2026-10-21 10:27:29 |
| Message-ID: | CAMPh8MrPMBnSYGmmqdp_pNesmupV5LS2pkBT-Zup34Ehs5d9kQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
thanks again for the review, Heikki.
> To find the logs, you have to check both places
>
> Also, if initdb succeeds but the operation fails later, there's no trace
> of the successful initdb in <new datadir>/pg_upgrade_output.d/
All logs, including initdb output, now go under
<new datadir>/pg_upgrade_output.d/<timestamp>/ from the beginning. To make
this possible, I followed your suggestion to let initdb accept
pg_upgrade_output.d in an otherwise empty target.
I considered using a temporary datadir, but initializing directly in the
final data directory seemed less intrusive. This way, moving initialized
files or rewriting configuration paths can be avoided and lets both upgrade
and check modes use the final data directory throughout.
The initdb output remains there if a later stage fails. Successful runs
use the existing log cleanup and --retain handling.
> We could modify initdb so that it ignores "pg_upgrade_output.d" and
> considers the target dir as empty as long as nothing else exists.
This approach is now implemented for both upgrade and check modes, with a
hidden --for-pg-upgrade=<state-file> option used only by pg_upgrade.
The state file stores cleanup information and is removed after a successful
run unless --retain is specified.
Without this option, initdb would reject the target because pg_upgrade has
already created pg_upgrade_output.d inside it. This option lets initdb
accept a target containing only pg_upgrade_output.d and preserve that log
directory if initialization fails. Other initialized files in the new
datadir follow initdb's normal failure cleanup.
The state file contains the external WAL path, whether the directory
existed before initdb, its original permissions if it existed, and its
nearest existing parent if new directories were needed. pg_upgrade uses
this information to remove generated WAL and restore the prior directory
state after --check, without duplicating initdb's option handling.
When running pg_upgrade --initdb, with or without --check, the workflow is:
phase 1. Check that new_datadir is empty or missing, allowing an existing
pg_upgrade_output.d directory. Create logs under
new_datadir/pg_upgrade_output.d/<timestamp>/.
phase 2. Run initdb directly in new_datadir with
--for-pg-upgrade=<state-file>. Here, <state-file> is
new_datadir/pg_upgrade_output.d/<timestamp>/log/initdb_state.
phase 3.1. With --check: read the state file and run the normal
compatibility
checks against the initialized cluster. After confirming shutdown,
remove initialized data and generated WAL, and restore prior target
and WAL directory existence and permissions. Cleanup runs whether
the compatibility checks succeed or fail.
phase 3.2. Without --check: run the normal compatibility checks against the
initialized cluster. If they pass, continue upgrading.
> I would expect "--check" to leave the directories unmodified. Except
> for the logs.
Agreed, after successful initialization, --check removes the initialized
data, except for the log, which is placed inside
new_datadir/pg_upgrade_output.d/<timestamp>/.
> With the above, the "<new dir>" would not have the right permissions
In the current patch, initdb now sets the target permissions itself, using
its
defaults or an explicit --allow-group-access. pg_upgrade applies matching
permissions
to newly created parents and logs, leaving existing parents unchanged.
> Step 3 is a bit scary. So we could still use a temporary dir in --check
> mode:
>
> 1. Create "<new dir>/pg_upgrade_output.d" and the log files
> 2. Run "initdb -D <new dir>/temporary_data_dir".
> 3. Delete "<new dir>/temporary_data_dir"
Right, the current patch's cleanup now requires successful initialization
and passing data-directory checks. If the state file cannot be read or
shutdown
is not confirmed, pg_upgrade reports failure and retains the cluster and
logs.
Data or WAL removal errors also report failure and preserve logs and any
remaining data.
When cleanup completes, the workflow meets all three goals you mentioned.
Additional tests were conducted besides the TAP tests
to ensure that running initdb manually followed by pg_upgrade produces
the same state after initialization and upgrade as pg_upgrade --initdb
with the same options. The test script is not included in the patch but is
available in the GitHub repository:
https://github.com/LeeBohyun/postgres/blob/pg_upgrade_initdb/pg-upgrade-initdb-parity.py
Best regards,
Bohyun
On Wed, Oct 14, 2026 at 7:54 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 14/10/2026 11:32, Bohyun Lee wrote:
> >> Why not write the logs under the new data directory, just like without
> >> --initdb?
> >
> > Agreed, the normal logs should end up in the new datadir, as they do
> > without --initdb.
> >
> > The patch captures early server and initdb output under
> >
> > ./pg_upgrade_output.d/<timestamp>/ in the current working directory,
> >
> > because initdb requires an empty target directory.
> >
> > But once initdb succeeds, it removes the temporary logs unless --retain
> > is specified.
> >
> > Subsequent output is written under
> >
> > <new datadir>/pg_upgrade_output.d/<timestamp>/ using the existing
> > cleanup and --retain handling.
> >
> > If initdb fails, pg_upgrade reports initialization failure and points to
> > the retained log.
>
> That sounds weird. To find the logs, you have to check both places; they
> might be under ./pg_upgrade_output.d/ or <new
> datadir>/pg_upgrade_output.d/ depending on the stage that it failed at.
> Also, if initdb succeeds but the operation fails later, there's no trace
> of the successful initdb in <new datadir>/pg_upgrade_output.d/, those
> logs are lost even though the logs for other stages are there.
>
> I see why you did it that way; there's a chicken-and-egg problem. In
> order to log the initdb invocation, the log dir must be created before
> initdb is launched, but initdb errors out if the target directory is not
> empty.
>
> We could modify initdb so that it ignores "pg_upgrade_output.d" and
> considers the target dir as empty as long as nothing else exists.
> Another idea is to run initdb in a subdir first, and rename it into the
> right place later (see below).
>
> >> But then you need write access to the current directory instead.
> >
> > pg_upgrade already requires a writable current directory, even without
> > --initdb.
> >
> > So, logging to a temporary location therefore add no new requirement
> > for write access to that directory.
>
> Oh, I see. (But see previous point).
>
> >>> --check --initdb retains the initialized new cluster whether the
> >>> compatibility
> >>> checks succeed or fail. After a successful check, rerun pg_upgrade
> >>> against that
> >
> >>> cluster without --check, --initdb, or --initdb-options.
> >
> >> Does that mean that if you run "pg_upgrade .. --check --initdb" twice,
> >> it will fail the second time because the new cluster already exists?
> >
> >> That doesn't sound nice.
> >
> > Right, repeated checks now reuse the existing initialized cluster rather
> > than failing.
>
> That's surprising too. I would expect "--check" to leave the directories
> unmodified. Except for the logs.
>
>
> Putting all this together, I think this would work (in non-check mode):
>
> 1. Check that "<new dir>" is empty or non-existent, like initdb does.
> (An existing "<new dir>/pg_upgrade_output.d" dir is acceptable though)
> 2. Create "<new dir>/pg_upgrade_output.d" and the log files
> 3. Run "initdb <new dir>/temporary_data_dir"
> 4. Move all contents of "<new dir>/temporary_data_dir" to "<new dir>"
> 5. Delete the now-empty "<new dir>/temporary_data_dir" directory.
>
> In --check mode, you can go through the same motions, except that in
> step 4 you can just delete "temporary_data_dir".
>
> One detail that's not quite right with this is the --allow-group-access
> again. With the above, the "<new dir>" would not have the right
> permissions, because pg_upgrade doesn't know the right permissions in
> step 1 yet. Maybe you could fix that up around steps 4-5: read the
> permissions of "<new dir>/temporary_data_dir" and copy them to "<new dir>".
>
>
> The other approach I mentioned above is to teach initdb to ignore
> "pg_upgrade_output.d". Then the sequence would be simply:
>
> 1. Create "<new dir>/pg_upgrade_output.d" and the log files
> 2. Run "initdb -D <new dir>". It ignores "pg_upgrade_output.d", but will
> complain if there's anything else in "<new dir>"
>
> In --check mode, it's a little more complicated though:
>
> 1. Create "<new dir>/pg_upgrade_output.d" and the log files
> 2. Run "initdb -D <new dir>". It ignores "pg_upgrade_output.d", but will
> complain if there's anything else in "<new dir>"
> 3. Delete everything under "<new dir>" except for "pg_upgrade_output.d"
>
> Step 3 is a bit scary. So we could still use a temporary dir in --check
> mode:
>
> 1. Create "<new dir>/pg_upgrade_output.d" and the log files
> 2. Run "initdb -D <new dir>/temporary_data_dir".
> 3. Delete "<new dir>/temporary_data_dir"
>
> The downside of that is that the --check mode diverges more from the
> real mode, so there's some risk of things working in --check mode, but
> failing when you run it for real.
>
> I'm not sure which of these approaches I like the best. What do you
> think? But I think the overall goals are:
>
> - the logs always go under "<new dir>/pg_upgrade_output.d"
> - the logs contain the initdb output too
> - --check mode leaves the target dir unmodified
>
>
> - Heikki
>
| Attachment | Content-Type | Size |
|---|---|---|
| v9-pg_upgrade-initdb.patch | application/octet-stream | 73.9 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Henson Choi | 2026-10-21 11:01:24 | [DEPARSE] AT TIME ZONE and similar as a function in FROM do not read back |
| Previous Message | Richard Guo | 2026-10-21 09:20:09 | Re: Wrong results from a mergejoin |