On Thu, Jul 27, 2023 at 6:46 AM Peter Smith <smithpb2...@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.