On Fri, Jul 21, 2023 at 5:24 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Jul 21, 2023 at 12:05 PM Peter Smith <smithpb2...@gmail.com> wrote: > > > > On Fri, Jul 21, 2023 at 3:39 PM Amit Kapila <amit.kapil...@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