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  (Peter Smith <smithpb2250@gmail.com>)
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication  (Peter Smith <smithpb2250@gmail.com>)
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication  (Peter Smith <smithpb2250@gmail.com>)
Список 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 по дате отправления:

Предыдущее
От: Amit Kapila
Дата:
Сообщение: Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
Следующее
От: "Jonathan S. Katz"
Дата:
Сообщение: Re: psql: Add role's membership options to the \du+ command