Re: Perform streaming logical transactions by background workers and parallel apply

On Wed, Sep 21, 2022 at 2:55 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> ======
>
> 3. .../replication/logical/applyparallelworker.c - parallel_apply_can_start
>
> +/*
> + * Returns true, if it is allowed to start a parallel apply worker, false,
> + * otherwise.
> + */
> +static bool
> +parallel_apply_can_start(TransactionId xid)
>
> Seems a slightly complicated comment for a simple boolean function.
>
> SUGGESTION
> Returns true/false if it is OK to start a parallel apply worker.
>

I think this is the style followed at some other places as well. So,
we can leave it.

>
> 6. src/backend/replication/logical/launcher.c - logicalrep_worker_detach
>
> logicalrep_worker_detach(void)
> {
> + /* Stop the parallel apply workers. */
> + if (!am_parallel_apply_worker() && !am_tablesync_worker())
> + {
> + List *workers;
> + ListCell *lc;
>
> The condition is not very obvious. This is why I previously suggested
> adding another macro/function like 'isLeaderApplyWorker'.
>

How about having function a function am_leader_apply_worker() { ...
return OidIsValid(MyLogicalRepWorker->relid) &&
(MyLogicalRepWorker->apply_leader_pid == InvalidPid) ...}?

>
> 13. src/include/replication/worker_internal.h
>
> + /*
> + * Indicates whether the worker is available to be used for parallel apply
> + * transaction?
> + */
> + bool in_use;
>
> This comment seems backward for this member's name.
>
> SUGGESTION (something like...)
> Indicates whether this ParallelApplyWorkerInfo is currently being used
> by a parallel apply worker processing a transaction. (If this flag is
> false then it means the ParallelApplyWorkerInfo is available for
> re-use by another parallel apply worker.)
>

I am not sure if this is an improvement over the current. The current
comment appears reasonable to me as it is easy to follow.

--
With Regards,
Amit Kapila.