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

Some review comments for v19-0001

======
src/backend/replication/logical/tablesync.c

1. run_tablesync_worker
+run_tablesync_worker(WalRcvStreamOptions *options,
+ char *slotname,
+ char *originname,
+ int originname_size,
+ XLogRecPtr *origin_startpos)
+{
+ /* Start table synchronization. */
+ start_table_sync(origin_startpos, &slotname);

There was no such comment ("/* Start table synchronization. */") in
the original HEAD code, so I didn't see that it adds much value by
adding it in the refactored code.

~~~

2. LogicalRepSyncTableStart

/*
* Finally, wait until the leader apply worker tells us to catch up and
* then return to let LogicalRepApplyLoop do it.
*/
wait_for_worker_state_change(SUBREL_STATE_CATCHUP);

~

Should LogicalRepApplyLoop still be mentioned here, since that is
static in worker.c? Maybe it is better to refer instead to the common
'start_apply' wrapper? (see also #5a below)

======
src/backend/replication/logical/worker.c

3. set_stream_options

+/*
+ * Sets streaming options including replication slot name and origin start
+ * position. Workers need these options for logical replication.
+ */
+void
+set_stream_options(WalRcvStreamOptions *options,

I'm not sure if the last sentence of the comment is adding anything useful.

~~~

4. start_apply
/*
* Run the apply loop with error handling. Disable the subscription,
* if necessary.
*
* Note that we don't handle FATAL errors which are probably because
* of system resource error and are not repeatable.
*/
void
start_apply(XLogRecPtr origin_startpos)

~

4a.
Somehow I found the function names to be confusing. Intuitively (IMO)
'start_apply' is for apply worker and 'start_tablesync' is for
tablesync worker. But actually, the start_apply() function is the
*common* function for both kinds of worker. Might be easier to
understand if start_apply function name can be changed to indicate it
is really common -- e.g. common_apply_loop(), or similar.

~

4b.
If adverse to changing the function name, it might be helpful anyway
if the function comment can emphasize this function is shared by
different worker types. e.g. "Common function to run the apply
loop..."

~~~

5. run_apply_worker

+ ReplicationOriginNameForLogicalRep(MySubscription->oid, InvalidOid,
+ originname, originname_size);
+
+ /* Setup replication origin tracking. */
+ StartTransactionCommand();

Even if you wish ReplicationOriginNameForLogicalRep() to be outside of
the transaction I thought it should still come *after* the comment,
same as it does in the HEAD code.

~~~

6. ApplyWorkerMain

- /* Run the main loop. */
- start_apply(origin_startpos);
+ /* This is leader apply worker */
+ run_apply_worker(&options, myslotname, originname,
sizeof(originname), &origin_startpos);

proc_exit(0);
}

~

6a.
The comment "/* This is leader apply worker */" is redundant now. This
function is the entry point for leader apply workers so it can't be
anything else.

~

6b.

Caller parameter wrapping differs from the similar code in
TablesyncWorkerMain. Shouldn't they be similar?

e.g.
+ run_apply_worker(&options, myslotname, originname,
sizeof(originname), &origin_startpos);

versus
+ run_tablesync_worker(&options,
+ myslotname,
+ originname,
+ sizeof(originname),
+ &origin_startpos);

======
src/include/replication/worker_internal.h

7.
+
+extern void set_stream_options(WalRcvStreamOptions *options,
+ char *slotname,
+ XLogRecPtr *origin_startpos);
+extern void start_apply(XLogRecPtr origin_startpos);
+extern void DisableSubscriptionAndExit(void);
+

Maybe all the externs belong together? It doesn't seem right for just
these 3 externs to be separated from all the others, with those static
inline functions in-between.

------
Kind Regards,
Peter Smith.
Fujitsu Australia