Re: Logical Replication of sequences

On Thu, Oct 16, 2025 at 4:54 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Tuesday, October 14, 2025 6:07 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > 2.
> > - /* Process any tables that are being synchronized in parallel. */
> > + /*
> > + * Process any tables that are being synchronized in parallel and any
> > + * newly added relations.
> > + */
> > ProcessSyncingRelations(commit_data.end_lsn);
> >
> > pgstat_report_activity(STATE_IDLE, NULL);
> > @@ -1364,7 +1372,10 @@ apply_handle_prepare(StringInfo s)
> >
> > in_remote_transaction = false;
> >
> > - /* Process any tables that are being synchronized in parallel. */
> > + /*
> > + * Process any tables that are being synchronized in parallel and any
> > + * newly added relations.
> > + */
> > ProcessSyncingRelations(prepare_data.end_lsn);
> >
> > In the first line of comment, it is mentioned as tables and in the
> > second line, the relations are mentioned. I think as part of this it
> > can process sequences as well if any are added. I wonder whether this
> > (while applying prepare/commit) is the right time to invoke it for
> > sequences. The apply worker do need to invoke sequencesync worker if
> > required but not sure if this is the right place.
>
> I agree that we can start sequence sync worker at any point regardless of the
> transaction boundary, because we do not support incremental seq sync, so another
> approach could be to call ProcessSyncingSequencesForApply() in the main loop of
> LogicalRepApplyLoop(), similar to maybe_advance_nonremovable_xid().
>
> OTOH, I think the current implementation also works, because we have one
> ProcessSyncingRelations() call in the main loop as well.
>

Yeah, the current implementation also appears good, but let's change
the comment to: "Process any tables that are being synchronized in
parallel, as well as any newly added tables or sequences."

> What do you think ?
>
> >
> > 4.
> > @@ -3284,7 +3307,7 @@ FindDeletedTupleInLocalRel(Relation localrel,
> > Oid localidxoid,
> > */
> > LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
> > leader = logicalrep_worker_find(MyLogicalRepWorker->subid,
> > - InvalidOid, false);
> > + InvalidOid, false, false);
> >
> > …
> > extern LogicalRepWorker *logicalrep_worker_find(Oid subid, Oid relid,
> > + LogicalRepWorkerType wtype,
> > bool only_running);
> >
> > The third parameter is LogicalRepWorkerType and passing it false in
> > the above usage doesn't make sense. Also, we should update comments
> > atop logicalrep_worker_find as to why worker_type is required. I want
> > to know why subid+relid combination is not sufficient to identify the
> > workers uniquely.
>
> I think it's because sequencesync worker also do not have a valid relid, similar
> to the apply worker, so we would need the worker type to distinguish them. I
> added some general comments atop of the function in the latest version.
>

Thanks, the added comments make the change easy to understand.

--
With Regards,
Amit Kapila.