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

On Fri, Jul 21, 2023 at 5:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 21, 2023 at 12:05 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Fri, Jul 21, 2023 at 3:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >

> > > 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.
> >
>
> Fair enough. We can do that as a first patch and then work on the
> refactoring patch to avoid introducing more inconsistencies or we can
> do the refactoring patch first but keep all the new function names to
> follow _ style.
>

Fixing the naming inconsistency will be more far-reaching than just a
few functions affected by these "reuse" patches. There are plenty of
existing functions already inconsistently named in the HEAD code. So
perhaps this topic should be moved to a separate thread?

For example, here are some existing/proposed names:

===

worker.c (HEAD)

static functions
  DisableSubscriptionAndExit -> disable_subscription_and_exit
  FindReplTupleInLocalRel -> find_repl_tuple_in_local_rel
  TwoPhaseTransactionGid -> two_phase_transaction_gid
  TargetPrivilegesCheck -> target_privileges_check
  UpdateWorkerStats -> update_worker_stats
  LogicalRepApplyLoop -> logical_rep_apply_loop

non-static functions
  stream_stop_internal -> StreamStopInternal
  apply_spooled_messages -> ApplySpooledMessages
  apply_dispatch -> ApplyDispatch
  store_flush_position -> StoreFlushPosition
  set_apply_error_context_origin -> SetApplyErrorContextOrigin

===

tablesync.c (HEAD)

static functions
  FetchTableStates -> fetch_table_states

non-static functions
  invalidate_syncing_table_states -> InvalidateSyncingTableStates

------
Kind Regards,
Peter Smith.
Fujitsu Australia