Hi, Attached the updated patches with recent reviews addressed.
See below for my comments:
Peter Smith <smithpb2...@gmail.com>, 19 Tem 2023 Çar, 06:08 tarihinde şunu
yazdı:
> Some review comments for v19-0001
>
> 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)
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.
Thanks,
--
Melih Mutlu
Microsoft
v21-0001-Refactor-to-split-Apply-and-Tablesync-Workers.patch
Description: Binary data
v21-0002-Reuse-Tablesync-Workers.patch
Description: Binary data
v21-0003-Reuse-connection-when-tablesync-workers-change-t.patch
Description: Binary data
