Обсуждение: Simplify some logical replication worker type checking
Hi hackers, BACKGROUND There are 3 different types of logical replication workers. 1. apply leader workers 2. parallel apply workers 3. tablesync workers And there are a number of places where the current worker type is checked using the am_<worker-type> inline functions. PROBLEM / SOLUTION During recent reviews, I noticed some of these conditions are a bit unusual. ====== 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); ~~~ 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. ------- Kind Regards, Peter Smith. Fujitsu Australia
Вложения
On Tuesday, August 1, 2023 9:36 AM Peter Smith <smithpb2250@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
On Tue, Aug 1, 2023 at 12:59 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote: > > > 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. > Hi Hou-san, Thanks for your review! Oops. Of course, you are right. My cut/paste typo was mostly confined to the post, but there was one instance also in the patch. Fixed in v2. ------ Kind Regards, Peter Smith. Fujitsu Australia
Вложения
On 2023-Aug-01, Peter Smith wrote: > PSA a small patch making those above-suggested changes. The 'make > check' and TAP subscription tests are all passing OK. I think the code ends up more readable with this style of changes, so +1. I do wonder if these calls should appear in a proc_exit callback or some such instead, though. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "No tengo por qué estar de acuerdo con lo que pienso" (Carlos Caszeli)
On Tue, Aug 1, 2023 at 12:11 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2023-Aug-01, Peter Smith wrote: > > > PSA a small patch making those above-suggested changes. The 'make > > check' and TAP subscription tests are all passing OK. > > I think the code ends up more readable with this style of changes, so > +1. I do wonder if these calls should appear in a proc_exit callback or > some such instead, though. > But the call to ApplyLauncherForgetWorkerStartTime()->logicalrep_launcher_attach_dshmem() has some dynamic shared memory allocation/attach calls which I am not sure is a good idea to do in proc_exit() callbacks. We may want to evaluate whether moving the suggested checks to proc_exit or any other callback is a better idea. What do you think? -- With Regards, Amit Kapila.
On Wed, Aug 2, 2023 at 8:20 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Aug 1, 2023 at 12:11 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > On 2023-Aug-01, Peter Smith wrote: > > > > > PSA a small patch making those above-suggested changes. The 'make > > > check' and TAP subscription tests are all passing OK. > > > > I think the code ends up more readable with this style of changes, so > > +1. I do wonder if these calls should appear in a proc_exit callback or > > some such instead, though. > > > > But the call to > ApplyLauncherForgetWorkerStartTime()->logicalrep_launcher_attach_dshmem() > has some dynamic shared memory allocation/attach calls which I am not > sure is a good idea to do in proc_exit() callbacks. We may want to > evaluate whether moving the suggested checks to proc_exit or any other > callback is a better idea. What do you think? > I have pushed the existing patch but feel free to pursue further improvements. -- With Regards, Amit Kapila.