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

On Tue, Jul 18, 2023 at 2:33 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
>
> Attached the fixed patchset.
>

Few comments on 0001
====================
1.
+ logicalrep_worker_attach(worker_slot);
+
+ /* Setup signal handling */
+ pqsignal(SIGHUP, SignalHandlerForConfigReload);
+ pqsignal(SIGTERM, die);
+ BackgroundWorkerUnblockSignals();
+
+ /*
+ * We don't currently need any ResourceOwner in a walreceiver process, but
+ * if we did, we could call CreateAuxProcessResourceOwner here.
+ */
+
+ /* Initialise stats to a sanish value */
+ MyLogicalRepWorker->last_send_time = MyLogicalRepWorker->last_recv_time =
+ MyLogicalRepWorker->reply_time = GetCurrentTimestamp();
+
+ /* Load the libpq-specific functions */
+ load_file("libpqwalreceiver", false);
+
+ InitializeLogRepWorker();
+
+ /* Connect to the origin and start the replication. */
+ elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
+ MySubscription->conninfo);
+
+ /*
+ * Setup callback for syscache so that we know when something changes in
+ * the subscription relation state.
+ */
+ CacheRegisterSyscacheCallback(SUBSCRIPTIONRELMAP,
+   invalidate_syncing_table_states,
+   (Datum) 0);

It seems this part of the code is the same for ApplyWorkerMain() and
TablesyncWorkerMain(). So, won't it be better to move it into a common
function?

2. Can LogicalRepSyncTableStart() be static function?

3. I think you don't need to send 0004, 0005 each time till we are
able to finish patches till 0003.

4. In 0001's commit message, you can say that it will help the
upcoming reuse tablesync worker patch.

--
With Regards,
Amit Kapila.