Re: Adding a LogicalRepWorker type field

Hi,

Peter Smith <smithpb2250(at)gmail(dot)com>, 2 Ağu 2023 Çar, 09:27 tarihinde şunu
yazdı:

> On Wed, Aug 2, 2023 at 3:35 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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,
--
Melih Mutlu
Microsoft