On Thu, Sep 4, 2025 at 9:51 AM vignesh C <[email protected]> wrote: > > On Wed, 3 Sept 2025 at 13:04, Hayato Kuroda (Fujitsu) > <[email protected]> wrote: > > > > Dear Vignesh, > > > > Thanks for updating the patch. Few comments: > > 01. > > ``` > > /* Find the leader apply worker and signal it. */ > > logicalrep_worker_wakeup(MyLogicalRepWorker->subid, InvalidOid); > > ``` > > > > Sequencesync worker does not need to send a signal to the apply worker. > > Should we skip in the case? > > Per my understanding, the signal is being used to set the status to > > STATE_READY. > > Modified > > > 02. > > ``` > > if (worker) > > worker->last_seqsync_start_time = 0; > > > > LWLockRelease(LogicalRepWorkerLock); > > ``` > > > > I feel we can release LWLock first then update last_seqsync_start_time. > > I felt it should be done within lock so that > ProcessSyncingSequencesForApply waits till the last_seqsync_start_time > is also set. > > > 03. > > Sequencesync worker cannot update its GUC parameters because > > ProcessConfigFile() > > is not called. How about checking the signal at the end of batch loop? > > Modified > > > 04. > > ``` > > while (search_pos < total_seqs) > > { > > LogicalRepSequenceInfo *candidate_seq = > > lfirst(list_nth_cell(sequences_to_copy, search_pos)); > > > > if (!strcmp(candidate_seq->nspname, > > nspname) && > > !strcmp(candidate_seq->seqname, > > seqname)) > > { > > seqinfo = candidate_seq; > > search_pos++; > > break; > > } > > > > search_pos++; > > } > > ``` > > > > It looks like that if the entry in sequences_to_copy is skipped, it won't be > > referred anymore. I feel this is method is bit dangerous, because ordering > > of > > the list may be different with the returned tuples from the publisher. > > Nodes may > > use the different collations. > > Modified > > The attached patch has the changes for the same.
Please rebase the patches as they conflict with current HEAD (due to commit 6359989654). Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
