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

From: "[email-masked]" <[email-masked]>
Date: Sat, 7 Jan 2023 05:42:59 +0000

On Saturday, January 7, 2023 12:50 PM Dilip Kumar <[email-masked]>
> 
> On Fri, Jan 6, 2023 at 3:38 PM [email-masked] <[email-masked]>
> wrote:
> >
> 
> Looks good, but I feel in pa_process_spooled_messages_if_required()
> function after getting the filestate the first check should be if (filestate==
> FS_EMPTY) return false.  I mean why to process through all the states if it is
> empty and we can directly exit.  It is not a big deal so if you prefer the way it is
> then I have no objection to it.

I think your suggestion looks good, I have adjusted the code.
I also rebase the patch set due to the recent commit c6e1f6.
And here is the new version patch set.

Best regards,
Hou zj

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 7 Jan 2023 14:25:31 +0530

> And here is the new version patch set.

LGTM

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

---

From: Amit Kapila <[email-masked]>
Date: Sun, 8 Jan 2023 07:44:03 +0530

On Sat, Jan 7, 2023 at 2:25 PM Dilip Kumar <[email-masked]> wrote:

Today, I was analyzing this patch w.r.t recent commit c6e1f62e2c and
found that pa_set_xact_state() should set the latch (wake up) for the
leader worker as the leader could be waiting in
pa_wait_for_xact_state(). What do you think? But otherwise, it should
be okay w.r.t DDLs because this patch allows the leader worker to
restart logical replication for subscription parameter change which
will in turn stop/restart parallel workers if required.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Sun, 8 Jan 2023 03:58:50 +0000

On Sunday, January 8, 2023 10:14 AM Amit Kapila <[email-masked]> wrote:
> Today, I was analyzing this patch w.r.t recent commit c6e1f62e2c and found that
> pa_set_xact_state() should set the latch (wake up) for the leader worker as the
> leader could be waiting in pa_wait_for_xact_state(). What do you think? But
> otherwise, it should be okay w.r.t DDLs because this patch allows the leader
> worker to restart logical replication for subscription parameter change which will
> in turn stop/restart parallel workers if required.

Thanks for the analysis. I agree that it would be better to signal the leader
when setting the state to PARALLEL_TRANS_STARTED, otherwise it might slightly
delay the timing of catch the state change in pa_wait_for_xact_state(), so I
have updated the patch for the same. Besides, I also checked commit c6e1f62e2c,
I think DDL operation doesn't need to wake up the parallel apply worker
directly as the parallel apply worker doesn't start table sync and only
communicate with the leader, so I didn't find some other places that need to be
changed.

Attach the updated patch set.

Best regards,
Hou zj

---

From: "[email-masked]" <[email-masked]>
Date: Sun, 8 Jan 2023 06:02:46 +0000

On Sunday, January 8, 2023 11:59 AM [email-masked] <[email-masked]> wrote:
> > found that
> > pa_set_xact_state() should set the latch (wake up) for the leader
> > worker as the leader could be waiting in pa_wait_for_xact_state().
> > What do you think? But otherwise, it should be okay w.r.t DDLs because
> > this patch allows the leader worker to restart logical replication for
> > subscription parameter change which will in turn stop/restart parallel workers
> if required.
> 
> Thanks for the analysis. I agree that it would be better to signal the leader when
> setting the state to PARALLEL_TRANS_STARTED, otherwise it might slightly delay
> the timing of catch the state change in pa_wait_for_xact_state(), so I have
> updated the patch for the same. Besides, I also checked commit c6e1f62e2c, I
> think DDL operation doesn't need to wake up the parallel apply worker directly
> as the parallel apply worker doesn't start table sync and only communicate with
> the leader, so I didn't find some other places that need to be changed.
> 

Sorry, the commit message of 0001 was accidentally deleted, just attach
the same patch set again with commit message.