From: | "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
Cc: | Bruce Momjian <bruce(at)momjian(dot)us>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: [PoC] pg_upgrade: allow to upgrade publisher node |
Date: | 2023-08-16 11:21:30 |
Message-ID: | OS0PR01MB5716BF8DE76B250661D0A4959415A@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
From | Date | Subject | |
---|---|---|---|
Next Message | Drouvot, Bertrand | 2023-08-16 11:43:35 | Re: WIP: new system catalog pg_wait_event |
Previous Message | Antonin Houska | 2023-08-16 11:20:23 | Re: walsender "wakeup storm" on PG16, likely because of bc971f4025c (Optimize walsender wake up logic using condition variables) |