RE: [PoC] pg_upgrade: allow to upgrade publisher node
От | Zhijie Hou (Fujitsu) |
---|---|
Тема | RE: [PoC] pg_upgrade: allow to upgrade publisher node |
Дата | |
Msg-id | OS0PR01MB5716BF8DE76B250661D0A4959415A@OS0PR01MB5716.jpnprd01.prod.outlook.com обсуждение исходный текст |
Ответ на | RE: [PoC] pg_upgrade: allow to upgrade publisher node ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>) |
Ответы |
Re: [PoC] pg_upgrade: allow to upgrade publisher node
(Amit Kapila <amit.kapila16@gmail.com>)
RE: [PoC] pg_upgrade: allow to upgrade publisher node ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>) |
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Antonin HouskaДата:
Сообщение: Re: walsender "wakeup storm" on PG16, likely because of bc971f4025c (Optimize walsender wake up logic using condition variables)