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
