RE: [PATCH] Reuse Workers and Replication Slots during Logical Replication - Mailing list pgsql-hackers

Dear Melih,

Thank you for making the patch!
I'm also interested in the patchset. Here are the comments for 0001.

Some codes are not suit for our coding conventions, but followings do not contain them
because patches seems in the early statge.
Moreover, 0003 needs rebase.

01. general

Why do tablesync workers have to disconnect from publisher for every iterations?
I think connection initiation overhead cannot be negligible in the postgres's basic
architecture. I have not checked yet, but could we add a new replication message
like STOP_STREAMING or CLEANUP? Or, engineerings for it is quite larger than the benefit?

02. logicalrep_worker_launch()

```
-       else
+       else if (!OidIsValid(relid))
                snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ApplyWorkerMain");
+       else
+               snprintf(bgw.bgw_function_name, BGW_MAXLEN, "TablesyncWorkerMain");
```

You changed the entry point of tablesync workers, but bgw_type is still the same.
Do you have any decisions about it? 

03. process_syncing_tables_for_sync()

```
+               /*
+                * Sync worker is cleaned at this point. It's ready to sync next table,
+                * if needed.
+                */
+               SpinLockAcquire(&MyLogicalRepWorker->relmutex);
+               MyLogicalRepWorker->ready_to_reuse = true;
+               SpinLockRelease(&MyLogicalRepWorker->relmutex);
```

Maybe acquiring the lock for modifying ready_to_reuse is not needed because all
the sync workers check only the own attribute. Moreover, other processes do not read.

04. sync_worker_exit()

```
+/*
+ * Exit routine for synchronization worker.
+ */
+void
+pg_attribute_noreturn()
+sync_worker_exit(void)
```

I think we do not have to rename the function from finish_sync_worker().

05. LogicalRepApplyLoop()

```
+                       if (MyLogicalRepWorker->ready_to_reuse)
+                       {
+                               endofstream = true;
+                       }
```

We should add comments here to clarify the reason.

06. stream_build_options()

I think we can set twophase attribute here.

07. TablesyncWorkerMain()

```
+       ListCell   *lc;
```

This variable should be declared inside the loop.

08. TablesyncWorkerMain()

```
+               /*
+                * If a relation with INIT state is assigned, clean up the worker for
+                * the next iteration.
+                *
+                * If there is no more work left for this worker, break the loop to
+                * exit.
+                */
+               if ( MyLogicalRepWorker->relstate == SUBREL_STATE_INIT)
+                       clean_sync_worker();
```

The sync worker sends a signal to its leader per the iteration, but it may be too
often. Maybe it is added for changing the rstate to READY, however, it is OK to
change it when the next change have come because should_apply_changes_for_rel()
returns true even if rel->state == SUBREL_STATE_SYNCDONE. I think the notification
should be done only at the end of sync workers. How do you think? 

Best Regards,
Hayato Kuroda
FUJITSU LIMITED