Re: Logical Replication of sequences

Hi Vignesh,

Some comments for v20250422-0004.

======
src/backend/commands/sequence.c

pg_sequence_state:

1.
+ * The page_lsn will be utilized in logical replication sequence
+ * synchronization to record the page_lsn of sequence in the
pg_subscription_rel
+ * system catalog. It will reflect the page_lsn of the remote sequence at the
+ * moment it was synchronized.
+ *

SUGGESTION (minor rewording)
The page LSN will be used in logical replication of sequences to
record the LSN of the sequence page in the pg_subscription_rel system
catalog. It reflects the LSN of the remote sequence at the time it
was synchronized.

======
src/backend/commands/subscriptioncmds.c

AlterSubscription:

2.
- case ALTER_SUBSCRIPTION_REFRESH:
+ case ALTER_SUBSCRIPTION_REFRESH_PUBLICATION_SEQUENCES:
+ {
+ if (!sub->enabled)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES is not
allowed for disabled subscriptions"));
+
+ PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ...
REFRESH PUBLICATION SEQUENCES");
+
+ AlterSubscription_refresh(sub, true, NULL, false, true, true);
+
+ break;
+ }
+
+ case ALTER_SUBSCRIPTION_REFRESH_PUBLICATION:

I felt these should be reordered so REFRESH PUBLICATION comes before
REFRESH PUBLICATION SEQUENCES. No particular reason, but AFAICT that
is how you've ordered them in all other places -- eg, gram.y, the
documentation, etc. -- so let's be consistent.

======
src/backend/replication/logical/launcher.c

3.
+/*
+ * Update the failure time of the sequencesync worker in the subscription's
+ * apply worker.
+ *
+ * This function is invoked when the sequencesync worker exits due to a
+ * failure.
+ */
+void
+logicalrep_seqsyncworker_failuretime(int code, Datum arg)

It might be better to call this function name
'logicalrep_seqsyncworker_failure' (not _failuretime) because it is
more generic, and in future, you might want to do more things in this
function apart from just setting the failure time.

======
src/backend/replication/logical/syncutils.c

SyncFinishWorker:

4.
+ /* This is a clean exit, so no need to set a sequence failure time. */
+ if (wtype == WORKERTYPE_SEQUENCESYNC)
+ cancel_before_shmem_exit(logicalrep_seqsyncworker_failuretime, 0);
+

I didn't think the comment should mention setting 'failure time'.
Those details belong at a lower level -- here, it is better to be more
generic.

SUGGESTION:
/* This is a clean exit, so no need for any sequence failure logic. */

======
Kind Regards,
Peter Smith.
Fujitsu Australia