/* * 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)
Isn't' LogicalRepApplyLoop static on HEAD and also mentioned in the exact comment in tablesync.c while the common "start_apply" function also exists? I'm not sure how such a change would be related to this patch.
---
5. + /* Found a table for next iteration */ + finish_sync_worker(true); + + StartTransactionCommand(); + ereport(LOG, + (errmsg("logical replication worker for subscription \"%s\" will be reused to sync table \"%s\" with relid %u.", + MySubscription->name, + get_rel_name(MyLogicalRepWorker->relid), + MyLogicalRepWorker->relid))); + CommitTransactionCommand(); + + done = false; + break; + } + LWLockRelease(LogicalRepWorkerLock);
5b. Isn't there a missing call to that LWLockRelease, if the 'break' happens?
Lock is already released before break, if that's the lock you meant:
/* Update worker state for the next table */ MyLogicalRepWorker->relid = rstate->relid; MyLogicalRepWorker->relstate = rstate->state; MyLogicalRepWorker->relstate_lsn = rstate->lsn; LWLockRelease(LogicalRepWorkerLock);
/* Found a table for next iteration */ finish_sync_worker(true); done = false; break;
---
2. As for the publisher node, this patch allows to reuse logical walsender processes after the streaming is done once.
~
Is this paragraph even needed? Since the connection is reused then it already implies the other end (the Wlasender) is being reused, right?
I actually see no harm in explaining this explicitly.
