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

On Fri, Jul 21, 2023 at 3:39 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Jul 21, 2023 at 7:30 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > ~~~
> >
> > 2. StartLogRepWorker
> >
> > /* Common function to start the leader apply or tablesync worker. */
> > void
> > StartLogRepWorker(int worker_slot)
> > {
> > /* Attach to slot */
> > logicalrep_worker_attach(worker_slot);
> >
> > /* Setup signal handling */
> > pqsignal(SIGHUP, SignalHandlerForConfigReload);
> > pqsignal(SIGTERM, die);
> > BackgroundWorkerUnblockSignals();
> >
> > /*
> > * We don't currently need any ResourceOwner in a walreceiver process, but
> > * if we did, we could call CreateAuxProcessResourceOwner here.
> > */
> >
> > /* Initialise stats to a sanish value */
> > MyLogicalRepWorker->last_send_time = MyLogicalRepWorker->last_recv_time =
> > MyLogicalRepWorker->reply_time = GetCurrentTimestamp();
> >
> > /* Load the libpq-specific functions */
> > load_file("libpqwalreceiver", false);
> >
> > InitializeLogRepWorker();
> >
> > /* Connect to the origin and start the replication. */
> > elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
> > MySubscription->conninfo);
> >
> > /*
> > * Setup callback for syscache so that we know when something changes in
> > * the subscription relation state.
> > */
> > CacheRegisterSyscacheCallback(SUBSCRIPTIONRELMAP,
> > invalidate_syncing_table_states,
> > (Datum) 0);
> > }
> >
> > ~
> >
> > 2a.
> > The function name seems a bit misleading because it is not really
> > "starting" anything here - it is just more "initialization" code,
> > right? Nor is it common to all kinds of LogRepWorker. Maybe the
> > function could be named something else like 'InitApplyOrSyncWorker()'.
> > -- see also #2c
> >
>
> How about SetupLogRepWorker?

The name is better than StartXXX, but still, SetupXXX seems a synonym
of InitXXX. That is why I thought it is a bit awkward having 2
functions with effectively the same name and the same
initialization/setup purpose (the only difference is one function
excludes parallel workers, and the other function is common to all
workers).

> The other thing I noticed is that we
> don't seem to be consistent in naming functions in these files. For
> example, shall we make all exposed functions follow camel case (like
> InitializeLogRepWorker) and static functions follow _ style (like
> run_apply_worker) or the other possibility is to use _ style for all
> functions except may be the entry functions like ApplyWorkerMain()? I
> don't know if there is already a pattern but if not then let's form it
> now, so that code looks consistent.
>

+1 for using some consistent rule, but I think this may result in
*many* changes, so it would be safer to itemize all the changes first,
just to make sure everybody is OK with it first before updating
everything.

------
Kind Regards,
Peter Smith