Re: Logical Replication of sequences

On Tue, Oct 14, 2025 at 3:33 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)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.

1. invalidate_syncing_table_states is changed to
InvalidateRelationStates. We could still retain syncing in it and name
it as InvalidateSyncingRelationStates

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.

3.
@@ -378,9 +378,6 @@ ProcessSyncingTablesForApply(XLogRecPtr current_lsn)

Assert(!IsTransactionState());

- /* We need up-to-date sync state info for subscription tables here. */
- FetchRelationStates(&started_tx);

I think it is better to let FetchRelationStates be invoked from here
as it sets the context of further work and makes it easy to understand
the code flow. We can even do the same for
ProcessSyncingSequencesForApply().

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.

--
With Regards,
Amit Kapila.