Here are some review comments for v22-0002 ====== 1. General - errmsg
AFAIK, the errmsg part does not need to be enclosed by extra parentheses. e.g. BEFORE ereport(LOG, (errmsg("logical replication table synchronization worker for subscription \"%s\" has finished", MySubscription->name))); AFTER ereport(LOG, errmsg("logical replication table synchronization worker for subscription \"%s\" has finished", MySubscription->name)); ~ The patch has multiple cases similar to that example. ====== src/backend/replication/logical/tablesync.c 2. + if (reuse_worker) + { + ereport(LOG, + (errmsg("logical replication table synchronization worker for subscription \"%s\" will be reused to sync table \"%s\" with relid %u.", + MySubscription->name, + get_rel_name(MyLogicalRepWorker->relid), + MyLogicalRepWorker->relid))); + } + else + { + ereport(LOG, + (errmsg("logical replication table synchronization worker for subscription \"%s\" has finished", + MySubscription->name))); + } These brackets { } are not really necessary. ~~~ 3. TablesyncWorkerMain + for (;!done;) + { + List *rstates; + ListCell *lc; + + run_tablesync_worker(); + + if (IsTransactionState()) + CommitTransactionCommand(); + + if (MyLogicalRepWorker->relsync_completed) + { + /* + * This tablesync worker is 'done' unless another table that needs + * syncing is found. + */ + done = true; Those variables 'rstates' and 'lc' do not need to be declared at this scope -- they can be declared further down, closer to where they are needed. ===== src/backend/replication/logical/worker.c 4. LogicalRepApplyLoop + + if (am_tablesync_worker()) + /* + * If relsync_completed is true, this means that the tablesync + * worker is done with synchronization. Streaming has already been + * ended by process_syncing_tables_for_sync. We should move to the + * next table if needed, or exit. + */ + if (MyLogicalRepWorker->relsync_completed) + endofstream = true; Here I think it is better to use bracketing { } for the outer "if", instead of only relying on the indentation for readability. YMMV. ------ Kind Regards, Peter Smith. Fujitsu Australia