Re: Adding a LogicalRepWorker type field

PSA v4 patches.

On Fri, Aug 4, 2023 at 5:45 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Thank you for the feedback received so far. Sorry, I have become busy
> lately with other work.
>
> IIUC there is a general +1 for the idea, but I still have some
> juggling necessary to make the functions/macros of patch 0001
> acceptable to everybody.
>
> The difficulties arise from:
> - no function overloading C
> - ideally, we prefer the same names for everything (e.g. instead of
> dual-set macros)
> - but launcher code calls need to pass param (other code always uses
> MyLogicalRepWorker)
> - would be nice (although no mandatory) to not have to always pass
> MyLogicalRepWorker as a param.
>
> Anyway, I will work towards finding some compromise on this next week.
> Currently, I feel the best choice is to go with what Bharath suggested
> in the first place -- just always pass the parameter (including
> passing MyLogicalRepWorker). I will think more about it...

v4-0001 uses only 3 simple inline functions. Callers always pass
parameters as Bharath had suggested.

>
> ------
>
> Meanwhile, here are some replies to other feedback received:
>
> Ashutosh -- "May be we should create a union of type specific members" [1].
>
> Yes, I was wondering this myself, but I won't pursue this yet until
> getting over the hurdle of finding acceptable functions/macros for
> patch 0001. Hopefully, we can come back to this idea.
>

To be explored later.

> ~~
>
> Mellih -- "Isn't the apply worker type still decided by eliminating
> the other choices even with the patch?".
>
> AFAIK the only case of that now is the one-time check in the
> logicalrep_worker_launch() function. IMO, that is quite a different
> proposition to the HEAD macros that have to make that deduction
> 10,000s ot times in executing code. Actually, even the caller knows
> exactly the kind of worker it wants to launch so we can pass the
> LogicalRepWorkerType directly to logicalrep_worker_launch() and
> eliminate even this one-time-check. I can code it that way in the next
> patch version.
>

Now even the one-time checking to assign the worker type is removed.
The callers know the LogicalReWorkerType they want, so v4-0001 just
passes the type into logicalrep_worker_launch()

> ~~
>
> Barath -- "-1 for these names starting with prefix TYPE_, in fact LR_
> looked better."
>
> Hmmm. I'm not sure what is best. Of the options below I prefer
> "WORKER_TYPE_xxx", but I don't mind so long as there is a consensus.
>
> LR_APPLY_WORKER
> LR_PARALLEL_APPLY_WORKER
> LR_TABLESYNC_WORKER
>
> TYPE_APPLY_WORKERT
> TYPE_PARALLEL_APPLY_WORKER
> TYPE_TABLESYNC_WORKER
>
> WORKER_TYPE_APPLY
> WORKER_TYPE_PARALLEL_APPLY
> WORKER_TYPE_TABLESYNC
>
> APPLY_WORKER
> PARALLEL_APPLY_WORKER
> TABLESYNC_WORKER
>
> APPLY
> PARALLEL_APPLY
> TABLESYNC
>

For now, in v4-0001 these are called WORKERTYPE_xxx. Please do propose
better names if these are no good.

------
Kind Regards,
Peter Smith.
Fujitsu Australia