RE: [PATCH] Reuse Workers and Replication Slots during Logical Replication

On Wed, Feb 1, 2023 20:07 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> Thanks for pointing out this review. I somehow skipped that, sorry.
> 
> Please see attached patches.

Thanks for updating the patch set.
Here are some comments.

1. In the function ApplyWorkerMain.
+			/* This is main apply worker */
+			run_apply_worker(&options, myslotname, originname, sizeof(originname), &origin_startpos);

I think we need to keep the worker name as "leader apply worker" in the comment
like the current HEAD.

---

2. In the function LogicalRepApplyLoop.
+				 * can be reused, we need to take care of memory contexts here
+				 * before moving to sync a table.
+				 */
+				if (MyLogicalRepWorker->ready_to_reuse)
+				{
+					MemoryContextResetAndDeleteChildren(ApplyMessageContext);
+					MemoryContextSwitchTo(TopMemoryContext);
+					return;
+				}

I think in this case we also need to pop the error context stack before
returning. Otherwise, I think we might use the wrong callback
(apply error_callback) after we return from this function.

---

3. About the function UpdateSubscriptionRelReplicationSlot.
This newly introduced function UpdateSubscriptionRelReplicationSlot does not
seem to be invoked. Do we need this function?

Regards,
Wang Wei