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

Hi,

Attached the updated patches with recent reviews addressed.

See below for my comments:

Peter Smith <smithpb2250@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