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

From Daniel Gustafsson
Subject Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically
Date
Msg-id 24958906-2301-478B-925F-7977E3785069@yesql.se
Whole thread
In response to Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically  (Bohyun Lee <bohyun.lee@databricks.com>)
Responses Re: [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically
List pgsql-hackers
I am not convinced that all this complexity and overhead isn't better suited
for external wrappers to pg_upgrade like Debian's pg_upgradecluster etc.

+    if (user_opts.initdb_new_cluster)
+        create_new_cluster_via_initdb();
+
     adjust_data_dir(&new_cluster);

If pg_upgrade controls how initdb was invoked for the new cluster, shouldn't it
work such that adjust_data_dir isn't required?


+    /*
+     * get_control_data() selects pg_resetwal vs. pg_resetxlog via
+     * bin_version, which check_bindir() normally fills in later.  Seed it now
+     * so the right binary name is used in this early call.
+     */
+    if (old_cluster.bin_version == 0)
+        old_cluster.bin_version = old_cluster.major_version;
+
+    get_control_data(&old_cluster);

This can't be done unconditionally, the old cluster can still be running at
this point.  For example if someone wants to do a live-check:

    $ ./bin/pg_upgrade -b ./bin/ -B ./bin/ -d ./data_old/ -D ./data_new/ --check --initdb

    The source cluster was not shut down cleanly, state reported as: "in production"
    Failure, exiting

get_control_data is big, expensive, and really designed to be run once.  I
don't think it's Ok to run it an extra time here without at least being able to
tell the later invocation that it has already been executed.  Also,
check_bindir() as referred to in the comment does not exist.


+ prep_status("Inspecting old cluster locale for new cluster creation");
+ start_postmaster(&old_cluster, true);
+ get_template0_info(&old_cluster);
+ stop_postmaster(false);
+ check_ok();

Again, cannot be done unconditionally.


+     * Users needing options that only the postmaster accepts can create the
+     * new cluster manually and omit --initdb.

This should probably be expanded upon in the documentation.

--
Daniel Gustafsson




pgsql-hackers by date:

Previous
From: Akshay Joshi
Date:
Subject: Re: [PATCH] Add pg_get_table_ddl() to reconstruct CREATE TABLE statements
Next
From: solai v
Date:
Subject: Re: Return pg_control from pg_backup_stop().