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

On Wed, Feb 1, 2023 20:07 PM Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)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