RE: Simplify some logical replication worker type checking - Mailing list pgsql-hackers

From Zhijie Hou (Fujitsu)
Subject RE: Simplify some logical replication worker type checking
Date
Msg-id OS0PR01MB5716E22C7664FDF62642A17B940AA@OS0PR01MB5716.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Simplify some logical replication worker type checking  (Peter Smith <smithpb2250@gmail.com>)
Responses Re: Simplify some logical replication worker type checking
List pgsql-hackers
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


pgsql-hackers by date:

Previous
From: Masahiro Ikeda
Date:
Subject: Re: Support to define custom wait events for extensions
Next
From: Amit Kapila
Date:
Subject: Re: logical decoding and replication of sequences, take 2