Misleading comment in pg_upgrade.c
Hello,
While reading pg_upgrade code to restore the objects on the new
cluster, I noticed that 5b570d771b8 didn't adjust the database name in
the comments explaining the requirements for an extra "--clean" for
template1 and postgres databases. While it's true that both databases
will already exist, I found it confusing to mention both names when
only one is processed for each code path.
Attachments:
fix_pgupgrade_comment.diffapplication/octet-stream; name=fix_pgupgrade_comment.diffDownload
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 5154a7160d..403f709afd 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -341,10 +341,9 @@ create_new_objects(void)
snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);
/*
- * template1 and postgres databases will already exist in the target
- * installation, so tell pg_restore to drop and recreate them;
- * otherwise we would fail to propagate their database-level
- * properties.
+ * template1 database will already exist in the target installation,
+ * so tell pg_restore to drop and recreate it; otherwise we would fail
+ * to propagate its database-level properties.
*/
create_opts = "--clean --create";
@@ -378,10 +377,9 @@ create_new_objects(void)
snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);
/*
- * template1 and postgres databases will already exist in the target
- * installation, so tell pg_restore to drop and recreate them;
- * otherwise we would fail to propagate their database-level
- * properties.
+ * postgres database will already exist in the target installation, so
+ * tell pg_restore to drop and recreate it; otherwise we would fail to
+ * propagate its database-level properties.
*/
if (strcmp(old_db->db_name, "postgres") == 0)
create_opts = "--clean --create";
On 5 Dec 2019, at 10:17, Julien Rouhaud <rjuju123@gmail.com> wrote:
While reading pg_upgrade code to restore the objects on the new
cluster, I noticed that 5b570d771b8 didn't adjust the database name in
the comments explaining the requirements for an extra "--clean" for
template1 and postgres databases. While it's true that both databases
will already exist, I found it confusing to mention both names when
only one is processed for each code path.
Agreed, I think this reads better.
cheers ./daniel
On Thu, Dec 5, 2019 at 11:45:09PM +0100, Daniel Gustafsson wrote:
On 5 Dec 2019, at 10:17, Julien Rouhaud <rjuju123@gmail.com> wrote:
While reading pg_upgrade code to restore the objects on the new
cluster, I noticed that 5b570d771b8 didn't adjust the database name in
the comments explaining the requirements for an extra "--clean" for
template1 and postgres databases. While it's true that both databases
will already exist, I found it confusing to mention both names when
only one is processed for each code path.Agreed, I think this reads better.
FYI, this patch was applied:
commit 690c880269
Author: Michael Paquier <michael@paquier.xyz>
Date: Fri Dec 6 11:55:04 2019 +0900
Improve some comments in pg_upgrade.c
When restoring database schemas on a new cluster, database "template1"
is processed first, followed by all other databases in parallel,
including "postgres". Both "postgres" and "template1" have some extra
handling to propagate each one's properties, but comments were confusing
regarding which one is processed where.
Author: Julien Rouhaud
Reviewed-by: Daniel Gustafsson
Discussion: /messages/by-id/CAOBaU_a2iviTG7FE10yO_gcW+zQCHNFhRA_NDiktf3UR65BHdw@mail.gmail.com
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +Le sam. 21 déc. 2019 à 18:46, Bruce Momjian <bruce@momjian.us> a écrit :
On Thu, Dec 5, 2019 at 11:45:09PM +0100, Daniel Gustafsson wrote:
On 5 Dec 2019, at 10:17, Julien Rouhaud <rjuju123@gmail.com> wrote:
While reading pg_upgrade code to restore the objects on the new
cluster, I noticed that 5b570d771b8 didn't adjust the database name in
the comments explaining the requirements for an extra "--clean" for
template1 and postgres databases. While it's true that both databases
will already exist, I found it confusing to mention both names when
only one is processed for each code path.Agreed, I think this reads better.
FYI, this patch was applied:
commit 690c880269
Author: Michael Paquier <michael@paquier.xyz>
Date: Fri Dec 6 11:55:04 2019 +0900Improve some comments in pg_upgrade.c
When restoring database schemas on a new cluster, database
"template1"
is processed first, followed by all other databases in
parallel,
including "postgres". Both "postgres" and "template1" have
some extra
handling to propagate each one's properties, but comments were
confusing
regarding which one is processed where.Author: Julien Rouhaud
Reviewed-by: Daniel Gustafsson
Discussion:
/messages/by-id/CAOBaU_a2iviTG7FE10yO_gcW+zQCHNFhRA_NDiktf3UR65BHdw@mail.gmail.com
Thanks Bruce, and thanks Michael for pushing!