RE: Perform streaming logical transactions by background workers and parallel apply

On Tuesday, January 17, 2023 2:46 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Tue, Jan 17, 2023 at 12:37 PM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > Attach the new version 0001 patch which addressed all other comments.
> >
>
> Thank you for updating the patch. Here is one comment:
>
> @@ -426,14 +427,24 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
>
> /*
> * Show the leader only for active parallel
> workers. This
> - * leaves the field as NULL for the
> leader of a parallel
> - * group.
> + * leaves the field as NULL for the
> leader of a parallel group
> + * or the leader of parallel apply workers.
> */
> if (leader && leader->pid !=
> beentry->st_procpid)
> {
> values[28] =
> Int32GetDatum(leader->pid);
> nulls[28] = false;
> }
> + else
> + {
> + int
> leader_pid = GetLeaderApplyWorkerPid(beentry->st_procpid);
> +
> + if (leader_pid != InvalidPid)
> + {
> + values[28] =
> Int32GetDatum(leader_pid);
> + nulls[28] = false;
> + }
> + }
> }
>
> I'm slightly concerned that there could be overhead of executing
> GetLeaderApplyWorkerPid () for every backend process except for parallel
> query workers. The number of such backends could be large and
> GetLeaderApplyWorkerPid() acquires the lwlock. For example, does it make
> sense to check (st_backendType == B_BG_WORKER) before calling
> GetLeaderApplyWorkerPid()? Or it might not be a problem since it's
> LogicalRepWorkerLock which is not likely to be contended.

Thanks for the comment and I think your suggestion makes sense.
I have added the check before getting the leader pid. Here is the new version patch.

Best regards,
Hou zj