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
