RE: Logical Replication of sequences - Mailing list pgsql-hackers

On Tuesday, October 14, 2025 6:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Tue, Oct 14, 2025 at 3:33 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > 0003~0005:
> > Unchanged.
> >
> > TODO:
> > * The latest comment from Shveta[5].
> > * The comment from Amit[6] to avoid creating slot/origin for sequence only
> subscription.
> >
> 
> Few comments on 0003 and 0004 based on previous version of the patch.
> As those are not changed, so I assume they apply for the new version
> as well.

Thanks for the comments.

> 
> 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.

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.

Best Regards,
Hou zj