Simplify some logical replication worker type checking

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.