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

On Wednesday, August 16, 2023 6:25 PM Kuroda, Hayato/黒田 隼人 wrote:
> 
> Dear hackers,
> 
> > > > It was primarily for upgrade purposes only. So, as we can't see a
> > > > good reason
> > to
> > > > go via pg_dump let's do it in upgrade unless someone thinks otherwise.
> > >
> > > Removed the new option in pg_dump and modified the pg_upgrade
> > > directly use the slot info to restore the slot in new cluster.
> >
> > In this version, creations of logical slots are serialized, whereas
> > old ones were parallelised per db. Do you it should be parallelized
> > again? I have tested locally and felt harmless. Also, this approch allows to log
> the executed SQLs.
> 
> I updated the patch to allow parallel executions. Workers are launched per
> slots, each one connects to the new node via psql and executes
> pg_create_logical_replication_slot().
> Moreover, following points were changed for 0002.
> 
> * Ensured to log executed SQLs for creating slots.
> * Fixed an issue that 'unreserved' slots could not be upgrade. This change was
>   not expected one. Related discussion was [1].
> * Added checks for output plugin libraries. pg_upgrade ensures that plugins
>   referred by old slots were installed to the new executable directory.


Thanks for updating the patch ! Here are few comments:

+static void
+create_logical_replication_slots(void)
...
+        query = createPQExpBuffer();
+        escaped = createPQExpBuffer();
+        conn = connectToServer(&new_cluster, old_db->db_name);

Since the connection here is not used anymore, so I think we can remove it.

2.

+static void
+create_logical_replication_slots(void)
...
+    /* update new_cluster info again */
+    get_logical_slot_infos(&new_cluster);
+}

Do we need to get new slots again after restoring ?

3.

+    snprintf(query, sizeof(query),
+             "SELECT slot_name, plugin, two_phase "
+             "FROM pg_catalog.pg_replication_slots "
+             "WHERE database = current_database() AND temporary = false "
+             "AND wal_status <> 'lost';");
+
+    res = executeQueryOrDie(conn, "%s", query);
+

Instead of building the query in a new variable, can we directly put the SQL in executeQueryOrDie()
e.g.
executeQueryOrDie(conn, "SELECT slot_name, plugin, two_phase ...");


4.
+int    num_slots_on_old_cluster;

Instead of a new global variable, would it be better to record this in the cluster info ?


5.

         char        sql_file_name[MAXPGPATH],
                     log_file_name[MAXPGPATH];
+
         DbInfo       *old_db = &old_cluster.dbarr.dbs[dbnum];

There is an extra change here.

6.
+    for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
..
+        /* reap all children */
+        while (reap_child(true) == true)
+            ;
+    }

Maybe we can move the "while (reap_child(true) == true)" out of the for() loop ?

Best Regards,
Hou zj