Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
| От | Melih Mutlu |
|---|---|
| Тема | Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication |
| Дата | |
| Msg-id | CAGPVpCSPK1ew4K17+wGrRtTk6s2nbFb8iuHd6dKZhcaGm+dGbA@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication (Amit Kapila <amit.kapila16@gmail.com>) |
| Ответы |
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication |
| Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления: