Re: [PATCH] Support automatic sequence replication

[shveta malik]

On Thu, Mar 5, 2026 at 8:16 AM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
>
> Here is V10 patch set which addressed all comments.
>

Thank You. Please find a few comments on 001:


1)
+ /*
+ * Skip synchronization if the current user does not have sufficient
+ * privileges to read the sequence data.
+ */
+ if (local_last_value == 0)
+ return COPYSEQ_INSUFFICIENT_PERM;

I don't think it is the right way to handle this. The local_last_value
can be genuinely 0 for some cases and we may end up giving the wrong
ERROR.

Try this:
CREATE SEQUENCE my_seq START WITH 0 INCREMENT BY 1 MINVALUE 0;
And then set-up pub-sub.

We get:
2026-03-05 08:57:39.591 IST [92281] WARNING:  insufficient privileges
on sequence ("public.my_seq")
2026-03-05 08:57:39.591 IST [92281] ERROR:  logical replication
sequence synchronization failed for subscription "subi1"

Either we shall move back the acl check to the caller of GetSequence
or pass the info of acl-check failure in  a new argument or return
value.

2)
+ /*
+ * For sequences in INIT state, always sync. Otherwise, for
+ * sequences in READY state, only sync if there's drift.
+ */
  if (sync_status == COPYSEQ_SUCCESS)
- sync_status = copy_sequence(seqinfo,
- sequence_rel->rd_rel->relowner);
+ sync_status = copy_sequence(seqinfo, sequence_rel);

We shall add such a comment atop copy_sequence as well. I am unsure if
we need it here or not.

3)
+ /* Sleep for the configured interval */
+ (void) WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
+ sleep_ms,
+ WAIT_EVENT_LOGICAL_SYNC_STATE_CHANGE);

I don't think this wait-event is appropriate. Unlike tablesync, we are
not waiting for any state change here. Shall we add a new one for our
case? How about WAIT_EVENT_LOGICAL_SEQSYNC_MAIN? Thoughts?

4)
+ relstate = subrel->srsubstate;

it will be good to move it just after below part:

/* Skip if the relation is not a sequence */


5)

  }
+ /* Check if there are any sequences. */
+ has_subsequences = (seq_states != NIL);

One blank line before new change will improve readability.

6)

 ##########
 ## ALTER SUBSCRIPTION ... REFRESH PUBLICATION should cause sync of new
-# sequences of the publisher, but changes to existing sequences should
-# not be synced.
+# sequences of the publisher.
 ##########

 # Create a new sequence 'regress_s2', and update existing sequence 'regress_s1'

Last comment needs to be changed. Remove this please:  'and update
existing sequence 'regress_s1''

thanks
Shveta