On Wed, Sep 21, 2022 at 2:55 PM Peter Smith <smithpb2...@gmail.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.