Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication

On Thu, Jul 27, 2023 at 6:46 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for v22-0003
>
> ======
>
> 1. ApplicationNameForTablesync
> +/*
> + * Determine the application_name for tablesync workers.
> + *
> + * Previously, the replication slot name was used as application_name. Since
> + * it's possible to reuse tablesync workers now, a tablesync worker can handle
> + * several different replication slots during its lifetime. Therefore, we
> + * cannot use the slot name as application_name anymore. Instead, the slot
> + * number of the tablesync worker is used as a part of the application_name.
> + *
> + * FIXME: if the tablesync worker starts to reuse the replication slot during
> + * synchronization, we should again use the replication slot name as
> + * application_name.
> + */
> +static void
> +ApplicationNameForTablesync(Oid suboid, int worker_slot,
> + char *application_name, Size szapp)
> +{
> + snprintf(application_name, szapp, "pg_%u_sync_%i_" UINT64_FORMAT, suboid,
> + worker_slot, GetSystemIdentifier());
> +}
>
> 1a.
> The intent of the "FIXME" comment was not clear. Is this some existing
> problem that needs addressing, or is this really more like just an
> "XXX" warning/note for the future, in case the tablesync logic
> changes?
>

This seems to be a Note for the future, so better to use XXX notation here.

> ~
>
> 1b.
> Since this is a new function, should it be named according to the
> convention for static functions?
>
> e.g.
> ApplicationNameForTablesync -> app_name_for_tablesync
>

I think for now let's follow the style for similar functions like
ReplicationOriginNameForLogicalRep() and
ReplicationSlotNameForTablesync().

--
With Regards,
Amit Kapila.