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

Amit Kapila <amit.kapila16@gmail.com>, 21 Tem 2023 Cum, 08:39 tarihinde
şunu yazdı:

> On Fri, Jul 21, 2023 at 7:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
> How about SetupLogRepWorker? 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.
>

I agree that these files have inconsistencies in naming things.
Most of the time I can't really figure out which naming convention I should
use. I try to name things by looking at other functions with similar
responsibilities.


> 3.
> >  extern void pa_xact_finish(ParallelApplyWorkerInfo *winfo,
> >      XLogRecPtr remote_lsn);
> > +extern void set_stream_options(WalRcvStreamOptions *options,
> > +    char *slotname,
> > +    XLogRecPtr *origin_startpos);
> > +
> > +extern void start_apply(XLogRecPtr origin_startpos);
> > +extern void DisableSubscriptionAndExit(void);
> > +extern void StartLogRepWorker(int worker_slot);
> >
> > This placement (esp. with the missing whitespace) seems to be grouping
> > the set_stream_options with the other 'pa' externs, which are all
> > under the comment "/* Parallel apply worker setup and interactions
> > */".
> >
> > Putting all these up near the other "extern void
> > InitializeLogRepWorker(void)" might be less ambiguous.
> >
>
> +1. Also, note that they should be in the same order as they are in .c
> files.
>

I did not realize the order is the same with .c files. Good to know. I'll
fix it along with other comments.

Thanks,
-- 
Melih Mutlu
Microsoft