On Tuesday, August 1, 2023 9:36 AM Peter Smith <smithpb2...@gmail.com> > PROBLEM / SOLUTION > > During recent reviews, I noticed some of these conditions are a bit unusual.
Thanks for the patch. > > ====== > worker.c > > 1. apply_worker_exit > > /* > * Reset the last-start time for this apply worker so that the launcher > * will restart it without waiting for wal_retrieve_retry_interval if the > * subscription is still active, and so that we won't leak that hash table > * entry if it isn't. > */ > if (!am_tablesync_worker()) > ApplyLauncherForgetWorkerStartTime(MyLogicalRepWorker->subid); > > ~ > > In this case, it cannot be a parallel apply worker (there is a check > prior), so IMO it is better to simplify the condition here to below. > This also makes the code consistent with all the subsequent > suggestions in this post. > > if (am_apply_leader_worker()) > ApplyLauncherForgetWorkerStartTime(MyLogicalRepWorker->subid); This change looks OK to me. > ~~~ > > 2. maybe_reread_subscription > > /* Ensure we remove no-longer-useful entry for worker's start time */ > if (!am_tablesync_worker() && !am_parallel_apply_worker()) > ApplyLauncherForgetWorkerStartTime(MyLogicalRepWorker->subid); > proc_exit(0); > > ~ > > Should simplify the above condition to say: > if (!am_leader_apply_worker()) > > ~~~ > > 3. InitializeApplyWorker > > /* Ensure we remove no-longer-useful entry for worker's start time */ > if (!am_tablesync_worker() && !am_parallel_apply_worker()) > ApplyLauncherForgetWorkerStartTime(MyLogicalRepWorker->subid); > proc_exit(0); > > ~ > > Ditto. Should simplify the above condition to say: > if (!am_leader_apply_worker()) > > ~~~ > > 4. DisableSubscriptionAndExit > > /* Ensure we remove no-longer-useful entry for worker's start time */ > if (!am_tablesync_worker() && !am_parallel_apply_worker()) > ApplyLauncherForgetWorkerStartTime(MyLogicalRepWorker->subid); > > ~ > > Ditto. Should simplify the above condition to say: > if (!am_leader_apply_worker()) > > ------ > > PSA a small patch making those above-suggested changes. The 'make > check' and TAP subscription tests are all passing OK. About 2,3,4, it seems you should use "if (am_leader_apply_worker())" instead of "if (!am_leader_apply_worker())" because only leader apply worker should invoke this function. Best Regards, Hou zj