On Tuesday, November 22, 2022 2:49 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com> > > Dear Nathan, > > > I think you are correct. I did it this way in v2. I've also moved > > the bulk of the logic to logical/worker.c. > > Thanks for updating! It becomes better. Further comments: > > 01. AlterSubscription() > > ``` > + LogicalRepWorkersWakeupAtCommit(subid); > + > ``` > > Currently subids will be recorded even if the subscription is not modified. > I think LogicalRepWorkersWakeupAtCommit() should be called inside the if > (update_tuple).
I think an exception would be REFRESH PULLICATION in which case update_tuple is false, but it seems better to wake up apply worker in this case as well, because the apply worker is also responsible to start table sync workers for newly subscribed tables(in process_syncing_tables()). Besides, it seems not a must to wake up apply worker for ALTER SKIP TRANSACTION, Although there might be no harm for waking up in this case. > > 02. LogicalRepWorkersWakeupAtCommit() > > ``` > + oldcxt = MemoryContextSwitchTo(TopTransactionContext); > + on_commit_wakeup_workers_subids = > lappend_oid(on_commit_wakeup_workers_subids, > + > subid); > ``` > > If the subscription is altered twice in the same transaction, the same subid > will > be recorded twice. > I'm not sure whether it may be caused some issued, but list_member_oid() can > be used to avoid that. +1, list_append_unique_oid might be better. Best regards, Hou zj
