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);
}
Regards,