Dear Ajin,
Thanks for updating the patch. Here are my comments.
01. start_sequence_sync()
```
/* Need to start transaction for cache lookup */
StartTransactionCommand();
```
Here, we must check additional parameter changes, such as conninfo and
passwordrequired.
Also, it's inefficient because transactions are started each time.
Can we re-use maybe_reread_subscription() here? Some parameters do not take
effect for the sequence sync worker, but it is OK to exit even if they
are changed. If we use the function, no need to include "storage/ipc.h".
02. match_previous_words
No need to remove "REFRESH SEQUENCES" anymore.
03. CopySeqResult
```
COPYSEQ_NOWORK,
```
It describes why the copying is skipped. How about "COPYSEQ_NO_DRIFT"?
04. LogicalRepSyncSequences()
```
oldctx = MemoryContextSwitchTo(ApplyContext);
seq = palloc0_object(LogicalRepSequenceInfo);
seq->localrelid = subrel->srrelid;
seq->nspname = get_namespace_name(RelationGetNamespace(sequence_rel));
seq->seqname = pstrdup(RelationGetRelationName(sequence_rel));
seq->relstate = relstate;
seqinfos = lappend(seqinfos, seq);
MemoryContextSwitchTo(oldctx);
```
ISTM they are palloc'd but not pfree'd.
Since the sequencesync worker now has a long lifetime, we must take care of the
memory allocation/freeing more carefully. How about introducing per-interaction
memory context like ApplyMessageContext?
05. LogicalRepApplyLoop()
MaybeLaunchSequenceSyncWorker() should be called more; otherwise, the
sequencesync
worker won't be launched if the worker always receives messages and WL_TIMEOUT
does
not happen. Can you add most of the places under
maybe_advance_nonremovable_xid()?
Personally considered, no need to add within `else if (c ==
PqReplMsg_PrimaryStatusUpdate)`
because it just consumes status updates from the primary.
06.
Not sure if the issue should be discussed here, but I found that sequences more
likely to go backward if users use sequences on the subscriber side.
Previously, the sync could happen based on the request, and users could
understand
the risk. But now everything would be done automatically, thus they may be
surprised more.
Should we consider some ratchet mechanisms, or retain it now because it's not
expected usage?
E.g., `nextval()` is called three times, and synchronization occurs between
them.
```
subscriber=# SELECT nextval('seq');
nextval
---------
2
(1 row)
subscriber=# SELECT nextval('seq');
nextval
---------
3
(1 row)
subscriber=# -- synchronization happened
subscriber=# SELECT nextval('seq');
nextval
---------
1
(1 row)
```
07.
Question: Can we introduce an intermediate state, such as SYNC, to clarify
whether synchronization is proceeding?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
