On Wed, Aug 16, 2023 at 3:55 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > > > 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().
>
Will it be beneficial for slots? Invoking a separate process each time
could be more costlier than slot creation. The other thing is during
slot creation, the snapbuild waits for parallel transactions to finish
so that can also hurt the patch. I think we can test it by having 50,
100, or 500 slots on the old cluster and see if doing parallel
execution for the creation of those on the new cluster has any benefit
over serial execution.
> 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.
>
I think this is a good idea but did you test it with out-of-core
plugins, if so, can you please share the results? Also, let's update
this information in docs as well.
Few minor comments
1. Why the patch updates the slots info at the end of
create_logical_replication_slots()? Can you please update the comments
for the same?
2.
@@ -36,6 +36,7 @@ generate_old_dump(void)
{
char sql_file_name[MAXPGPATH],
log_file_name[MAXPGPATH];
+
DbInfo *old_db = &old_cluster.dbarr.dbs[dbnum];
Spurious line change.
--
With Regards,
Amit Kapila.