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