On Thu, Sep 4, 2025 at 9:51 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 3 Sept 2025 at 13:04, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> 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
