On Wed, Aug 2, 2023 at 3:35 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > I can see the problem you stated and it's true that the worker's type > never changes during its lifetime. But I'm not sure we really need to > add a new variable to LogicalRepWorker since we already have enough > information there to distinguish the worker's type. > > Currently, we deduce the worker's type by checking some fields that > never change such as relid and leader_piid. It's fine to me as long as > we encapcel the deduction of worker's type by macros (or inline > functions). Even with the proposed patch (0001 patch), we still need a > similar logic to determine the worker's type.
Thanks for the feedback.
I agree that the calling code will not look very different with/without this patch 0001, because everything is hidden by the macros/functions. But those HEAD macros/functions are also hiding the inefficiencies of the type deduction -- e.g. IMO it is quite strange that we can only recognize the worker is an "apply worker" by eliminating the other 2 possibilities.
Isn't the apply worker type still decided by eliminating the other choices even with the patch?
/* Prepare the worker slot. */ + if (OidIsValid(relid)) + worker->type = TYPE_TABLESYNC_WORKER; + else if (is_parallel_apply_worker) + worker->type = TYPE_PARALLEL_APPLY_WORKER; + else
+ worker->type = TYPE_APPLY_WORKER;
> 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. >
I agree that having both is_worker_type and IsXXXWorker makes the code more confusing. Especially both type check ways are used in the same file/function as follows:
logicalrep_worker_detach(void) { /* Stop the parallel apply workers. */ - if (am_leader_apply_worker()) + if (IsLeaderApplyWorker()) { List *workers; ListCell *lc; @@ -749,7 +756,7 @@ logicalrep_worker_detach(void) { LogicalRepWorker *w = (LogicalRepWorker *) lfirst(lc);
- if (isParallelApplyWorker(w)) + if (is_worker_type(w, TYPE_PARALLEL_APPLY_WORKER)) logicalrep_worker_stop_internal(w, SIGTERM); }