On Mon, 5 Aug 2024 at 17:28, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Aug 5, 2024 at 2:36 PM vignesh C <vignes...@gmail.com> wrote: > > > > On Fri, 2 Aug 2024 at 14:24, shveta malik <shveta.ma...@gmail.com> wrote: > > > > > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik <shveta.ma...@gmail.com> > > > wrote: > > > > > > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C <vignes...@gmail.com> wrote: > > > > > > > > > > Thanks for reporting this, these issues are fixed in the attached > > > > > v20240730_2 version patch. > > > > > > > > > > > I was reviewing the design of patch003, and I have a query. Do we need > > > to even start an apply worker and create replication slot when > > > subscription created is for 'sequences only'? IIUC, currently logical > > > replication apply worker is the one launching sequence-sync worker > > > whenever needed. I think it should be the launcher doing this job and > > > thus apply worker may even not be needed for current functionality of > > > sequence sync? > > > > But that would lead to maintaining all sequence-sync of each > subscription by launcher. Say there are 100 sequences per subscription > and some of them from each subscription are failing due to some > reasons then the launcher will be responsible for ensuring all the > sequences are synced. I think it would be better to handle > per-subscription work by the apply worker. > > > > > Going forward when we implement incremental sync of > > > sequences, then we may have apply worker started but now it is not > > > needed. > > > > I believe the current method of having the apply worker initiate the > > sequence sync worker is advantageous for several reasons: > > a) Reduces Launcher Load: This approach prevents overloading the > > launcher, which must handle various other subscription requests. > > b) Facilitates Incremental Sync: It provides a more straightforward > > path to extend support for incremental sequence synchronization. > > c) Reuses Existing Code: It leverages the existing tablesync worker > > code for starting the tablesync process, avoiding the need to > > duplicate code in the launcher. > > d) Simplified Code Maintenance: Centralizing sequence synchronization > > logic within the apply worker can simplify code maintenance and > > updates, as changes will only need to be made in one place rather than > > across multiple components. > > e) Better Monitoring and Debugging: With sequence synchronization > > being handled by the apply worker, you can more effectively monitor > > and debug synchronization processes since all related operations are > > managed by a single component. > > > > Also, I noticed that even when a publication has no tables, we create > > replication slot and start apply worker. > > > > As far as I understand slots and origins are primarily required for > incremental sync. Would it be used only for sequence-sync cases? If > not then we can avoid creating those. I agree that it would add some > complexity to the code with sequence-specific checks, so we can create > a top-up patch for this if required and evaluate its complexity versus > the benefit it produces.
I have added a XXX todo comments in the v20240807 version patch attached at [1]. I will handle this as a separate patch once the current patch is stable. [1] - https://www.postgresql.org/message-id/CALDaNm01Z6Oo9osGMFTOoyTR1kVoyh1rEvZ%2B6uJn-ZymV%3D0dbQ%40mail.gmail.com Regards, Vignesh
