Re: Logical Replication of sequences

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