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.
Regards,
Vignesh
