Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically - Mailing list pgsql-hackers
| From | Bohyun Lee |
|---|---|
| Subject | Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically |
| Date | |
| Msg-id | CAMPh8Mp7aH_iC_xMOOzeiB+kSGh5bAg4oRRS=H5KsC5a0x9iKQ@mail.gmail.com Whole thread |
| In response to | Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically (Greg Sabino Mullane <htamfids@gmail.com>) |
| Responses |
Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically
|
| List | pgsql-hackers |
Thanks for the review, Greg. Glad it's useful, and thanks for trying it across the version range.
Attached is v2, which addresses your points:
- Added test coverage for the settings the feature is really about. The test now initdb's the old cluster with data checksums and a non-default WAL segment size, and after the upgrade checks that the new cluster inherited the checksum setting, WAL segment size, encoding, collation, ctype, and locale provider.
- The overwrite test now confirms the failure comes from pg_upgrade's own PG_VERSION check ("already contains a database system") rather than initdb's non-empty-directory error.
- Added a test for the "initdb not present" case (pointing --new-bindir at an empty dir), since it was easy enough to cover.
- Reworked the -O handling. Rather than dropping it entirely, I took your alternative: only "-c name=value" options are forwarded to initdb now, and anything else is skipped with a warning. So -O "-c work_mem=..." works, and -O "-B 12345" no longer breaks initdb.
That keeps GUC pass-through working without pretending initdb accepts the full postmaster option set.
- Moved the "refuse to overwrite" check up to right after the initdb-present check.
- Fixed the copyright year on the new test file.
On the v10 failure ("invalid connection option max_protocol_version"): that comes from stock pg_upgrade, which adds max_protocol_version=3.0 to the connection string for pre-PG11 servers (server.c). It's not introduced by --initdb, the same connection is made in the normal flow, so I've left it out of scope for this patch.
The patch is pgindent-clean and the TAP test passes. For convenience, the branch is also on GitHub:
github.com/LeeBohyun/postgres/tree/pg_upgrade_initdb
Best regards,
Bohyun
My review. Overall, this looks really nice and a much needed feature. I will definitely be using this a lot in the future :)
It cleanly compiled on top of 97a18c22 (from Fri Jul 10 22:34:24 2026), and I was able to successfully do the upgrade via the new --initdb flag for versions 11 through 18. Version 10 did not work, but that is not the fault of this patch. But the output for completeness, since we still support upgrading from v10:
Inspecting old cluster locale for new cluster creation
invalid connection option "max_protocol_version"
I also verified that it correctly maintains the checksum setting, the locale, weird WAL segment sizes, and the new icu and builtin locale options. However, the tests do none of those checks, and they really ought to, as that's an important part of what this feature provides.
> Extra initdb options can be passed via the existing -O flag and will be
> forwarded to the initdb invocation.
I don't like this part, as there is not a 100% overlap between what postmaster accepts and what initdb accepts. The -c options are fine, but things like -O "-B 12345" cause a failure. I'd simply not add the -O options here: if people really need them, they can run pg_upgrade without the --initdb flag. Alternatively, only allow -c items, but that's a real pain to code that.
> +static void create_new_cluster_via_initdb(void);
Nice, I love descriptive function names.
> + * resolve_new_bindir()
Good idea, well executed.
> +/* Refuse to overwrite an existing cluster. */
Move this up to just after the other check that initdb is present?
> + snprintf(verfile, sizeof(verfile), "%s/PG_VERSION",
I suppose this is slightly better than just checking if the dir is empty or not, as initdb itself will later fail if non-empty but does not contain PG_VERSION. So a weak +1. Sort of has test coverage (but see below)
> + * Verify that initdb is present and executable before doing any work.
Also not tested at all (may be overkill to do so, but pointing out for completeness)
> +# Copyright (c) 2022-2025, PostgreSQL Global Development Group
s/2025/2026/ - or perhaps just say "2026" as this is a new file
> +# --initdb must refuse to clobber an already-populated data directory.
> +command_fails(
Would be nice if we confirmed this was failing due to the new PG_VERSION check, versus the initdb "ain't gonna write to non empty dir" error.
Cheers,
Greg
Attachment
pgsql-hackers by date: