Re: [PoC] pg_upgrade: allow to upgrade publisher node

On Fri, Sep 1, 2023 at 9:47 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Aug 31, 2023 at 7:56 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
>
Some more comments on 0002

1.
+ conn = connectToServer(&new_cluster, "template1");
+
+ prep_status("Checking for logical replication slots");
+
+ res = executeQueryOrDie(conn, "SELECT slot_name "
+ "FROM pg_catalog.pg_replication_slots "
+ "WHERE slot_type = 'logical' AND "
+ "temporary IS FALSE;");


I think we should add some comment saying this query will only fetch
logical slots because the database name will always be NULL in the
physical slots.  Otherwise looking at the query it is very confusing
how it is avoiding the physical slots.

2.
+void
+get_old_cluster_logical_slot_infos(void)
+{
+ int dbnum;
+
+ /* Logical slots can be migrated since PG17. */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
+ return;
+
+ pg_log(PG_VERBOSE, "\nsource databases:");

I think we need to change some headings like "slot info source
databases:"  Or add an extra message saying printing slot information.

Before this patch, we were printing all the relation information so
message ordering was quite clear e.g.

source databases:
Database: "template1"
relname: "pg_catalog.pg_largeobject", reloid: 2613, reltblspace: ""
relname: "pg_catalog.pg_largeobject_loid_pn_index", reloid: 2683,
reltblspace: ""
Database: "postgres"
relname: "pg_catalog.pg_largeobject", reloid: 2613, reltblspace: ""
relname: "pg_catalog.pg_largeobject_loid_pn_index", reloid: 2683,
reltblspace: ""

But after this patch slot information is also getting printed in a
similar fashion so it's very confusing now.  Refer
get_db_and_rel_infos() for how it is fetching all the relation
information first and then printing them.




3. One more problem is that the slot information and the execute query
messages are intermingled so it becomes more confusing, see the below
example of the latest messaging.  I think ideally we should execute
these queries first
and then print all slot information together instead of intermingling
the messages.

source databases:
executing: SELECT pg_catalog.set_config('search_path', '', false);
executing: SELECT slot_name, plugin, two_phase FROM
pg_catalog.pg_replication_slots WHERE wal_status <> 'lost' AND
database = current_database() AND temporary IS FALSE;
Database: "template1"
executing: SELECT pg_catalog.set_config('search_path', '', false);
executing: SELECT slot_name, plugin, two_phase FROM
pg_catalog.pg_replication_slots WHERE wal_status <> 'lost' AND
database = current_database() AND temporary IS FALSE;
Database: "postgres"
slotname: "isolation_slot1", plugin: "pgoutput", two_phase: 0

4.  Looking at the above two comments I feel that now the order should be like
- Fetch all the db infos
get_db_infos()
- loop
   get_rel_infos()
   get_old_cluster_logical_slot_infos()

-- and now print relation and slot information per database
 print_db_infos()

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com