Re: Adding a LogicalRepWorker type field

On Wed, Aug 2, 2023 at 2:46 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, Aug 2, 2023 at 12:14 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > We can't use the same names for both with/without-parameter functions
> > because there is no function overloading in C. In patch v3-0001 I've
> > replaced the "dual set of macros", with a single inline function of a
> > different name, and one set of space-saving macros.
> >
> > PSA v3
>
> Quick thoughts:
>
> 1.
> +typedef enum LogicalRepWorkerType
> +{
> + TYPE_UNKNOWN = 0,
> + TYPE_TABLESYNC_WORKER,
> + TYPE_APPLY_WORKER,
> + TYPE_PARALLEL_APPLY_WORKER
> +} LogicalRepWorkerType;
>
> -1 for these names starting with prefix TYPE_, in fact LR_ looked better.
>
> 2.
> +is_worker_type(LogicalRepWorker *worker, LogicalRepWorkerType type)
>
> This function name looks too generic, an element of logical
> replication is better in the name.
>
> 3.
> +#define IsLeaderApplyWorker() is_worker_type(MyLogicalRepWorker,
> TYPE_APPLY_WORKER)
> +#define IsParallelApplyWorker() is_worker_type(MyLogicalRepWorker,
> TYPE_PARALLEL_APPLY_WORKER)
> +#define IsTablesyncWorker() is_worker_type(MyLogicalRepWorker,
> TYPE_TABLESYNC_WORKER)
>
> My thoughts were around removing am_XXX_worker() and
> IsXXXWorker(&worker) functions and just have IsXXXWorker(&worker)
> alone with using IsXXXWorker(MyLogicalRepWorker) in places where
> am_XXX_worker() is used. If implementing this leads to something like
> the above with is_worker_type, -1 from me.
>

What I was suggesting to Peter to have something like:
static inline bool
am_tablesync_worker(void)
{
return (MyLogicalRepWorker->type == TYPE_APPLY_WORKER);
}

--
With Regards,
Amit Kapila.